Skip to content

refactor(csharp): cleanup tcp connection after vsr implementation - #3858

Open
lukaszzborek wants to merge 10 commits into
masterfrom
dotnet-vsr-cleanup
Open

refactor(csharp): cleanup tcp connection after vsr implementation#3858
lukaszzborek wants to merge 10 commits into
masterfrom
dotnet-vsr-cleanup

Conversation

@lukaszzborek

Copy link
Copy Markdown
Contributor

Follow-up cleanup of the .NET SDK TCP transport after the VSR support landed

@github-actions github-actions Bot added the S-waiting-on-review PR is waiting on a reviewer label Aug 10, 2026
@codecov

codecov Bot commented Aug 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 66.82135% with 143 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.93%. Comparing base (fb9307e) to head (aae1d4b).

Files with missing lines Patch % Lines
...SDK/IggyClient/Implementations/TcpMessageStream.cs 57.01% 40 Missing and 9 partials ⚠️
foreign/csharp/Iggy_SDK/Vsr/VsrConnection.cs 72.77% 40 Missing and 9 partials ⚠️
...IggyClient/Implementations/TcpMessageStream.Vsr.cs 53.62% 24 Missing and 8 partials ⚠️
foreign/csharp/Iggy_SDK/Consumers/IggyConsumer.cs 85.10% 2 Missing and 5 partials ⚠️
foreign/csharp/Iggy_SDK/Utils/ArrayPoolHelper.cs 75.00% 2 Missing and 1 partial ⚠️
...n/csharp/Iggy_SDK/Consumers/IggyConsumer.Rented.cs 0.00% 1 Missing and 1 partial ⚠️
foreign/csharp/Iggy_SDK/Mappers/BinaryMapper.cs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3858      +/-   ##
============================================
- Coverage     82.84%   81.93%   -0.92%     
  Complexity     1299     1299              
============================================
  Files          1199     1200       +1     
  Lines        161885   161835      -50     
  Branches     131360   131473     +113     
============================================
- Hits         134120   132595    -1525     
- Misses        24225    25644    +1419     
- Partials       3540     3596      +56     
Components Coverage Δ
Rust Core 83.36% <ø> (ø)
Java SDK 66.06% <ø> (ø)
C# SDK 56.94% <66.82%> (-19.03%) ⬇️
Python SDK 89.98% <ø> (ø)
PHP SDK 84.26% <ø> (ø)
Node SDK 96.21% <ø> (ø)
Go SDK 68.53% <ø> (ø)
Files with missing lines Coverage Δ
...n/csharp/Iggy_SDK/Utils/TcpMessageStreamHelpers.cs 91.42% <ø> (+6.64%) ⬆️
foreign/csharp/Iggy_SDK/Vsr/ConsensusSession.cs 98.98% <100.00%> (-1.02%) ⬇️
foreign/csharp/Iggy_SDK/Mappers/BinaryMapper.cs 82.10% <0.00%> (-11.39%) ⬇️
...n/csharp/Iggy_SDK/Consumers/IggyConsumer.Rented.cs 60.86% <0.00%> (-12.82%) ⬇️
foreign/csharp/Iggy_SDK/Utils/ArrayPoolHelper.cs 75.00% <75.00%> (ø)
foreign/csharp/Iggy_SDK/Consumers/IggyConsumer.cs 50.00% <85.10%> (-20.92%) ⬇️
...IggyClient/Implementations/TcpMessageStream.Vsr.cs 59.50% <53.62%> (-15.94%) ⬇️
...SDK/IggyClient/Implementations/TcpMessageStream.cs 51.10% <57.01%> (-32.06%) ⬇️
foreign/csharp/Iggy_SDK/Vsr/VsrConnection.cs 72.77% <72.77%> (ø)

... and 56 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@lukaszzborek

Copy link
Copy Markdown
Contributor Author

If possible, merge #3836 first. Then I will remove things with namespace which I refactoring 😅

@hubcio hubcio left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

solid direction overall - classic framing gone, the VsrConnection extraction, typed namespaces and the epoch rejoin are real improvements. blocking items are the probe losing its reconnect, the never-nulled _connection, the fail-open namespace and the edge-based Disconnected clear; the rest is small.

one thing outside the diff: SetConnectionStateAsync (TcpMessageStream.cs:1153) is a non-atomic check-then-set with the publish after the write, reachable from callers holding different locks - concurrent transitions can publish duplicates or a PreviousState that never actually preceded CurrentState. it predates this PR, and fixing it properly means moving the publish behind an ordered queue (holding a lock across the publish is not an option - the handler chain runs synchronously into socket I/O when the semaphores are uncontended, so a sync write fault re-enters the same lock). follow-up material, but it is the substrate under the Disconnected-clear race flagged inline.

Comment thread foreign/csharp/Iggy_SDK/Vsr/VsrNamespace.cs Outdated
Comment thread foreign/csharp/Iggy_SDK/Consumers/IggyConsumer.cs Outdated
Comment thread foreign/csharp/Iggy_SDK/IggyClient/Implementations/TcpMessageStream.Vsr.cs Outdated
Comment thread foreign/csharp/Iggy_SDK/Vsr/VsrConnection.cs Outdated
Comment thread foreign/csharp/Iggy_SDK/Vsr/VsrConnection.cs Outdated
Comment thread foreign/csharp/Iggy_SDK/IggyClient/Implementations/TcpMessageStream.cs Outdated
Comment thread foreign/csharp/Iggy_SDK/Consumers/IggyConsumer.cs
@github-actions github-actions Bot added S-waiting-on-author PR is waiting on author response and removed S-waiting-on-review PR is waiting on a reviewer labels Aug 11, 2026
# Conflicts:
#	foreign/csharp/Iggy_SDK/Vsr/VsrHeader.cs
#	foreign/csharp/Iggy_SDK/Vsr/VsrNamespace.cs
#	foreign/csharp/Iggy_SDK_Tests/VsrTests/VsrHeaderTests.cs
#	foreign/csharp/Iggy_SDK_Tests/VsrTests/VsrNamespaceTests.cs
# Conflicts:
#	foreign/csharp/Iggy_SDK/IggyClient/Implementations/TcpMessageStream.cs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-author PR is waiting on author response

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants