preserve unread data across detach by interface95 · Pull Request #831 · kerryjiang/SuperSocket

interface95 · 2026-05-25T16:43:55Z

Summary

Preserve unread bytes after a yielded package so detach handoff does not let the old pipeline consume raw data from the same buffer.
Add regression coverage for detach preserving buffered data after the current package.
Add a Kestrel HTTP/2 detach demo with no-signal-wait, stress, and TCP comparison modes.

Verification

dotnet test test/SuperSocket.Tests/SuperSocket.Tests.csproj --filter FullyQualifiedName~PipeConnectionBaseTest --logger "console;verbosity=minimal"
dotnet test test/SuperSocket.Tests/SuperSocket.Tests.csproj --filter FullyQualifiedName~ClientTest.TestDetachableConnection --logger "console;verbosity=detailed"
dotnet build -c Release
dotnet test test/SuperSocket.Tests/SuperSocket.Tests.csproj --no-build --logger "console;verbosity=minimal"
dotnet run --project samples/KestrelHttp2DetachDemo/KestrelHttp2DetachDemo.csproj -c Release -- --no-signal-wait
dotnet run --project samples/KestrelHttp2DetachDemo/KestrelHttp2DetachDemo.csproj -c Release -- --compare-tcp
dotnet run --project samples/KestrelHttp2DetachDemo/KestrelHttp2DetachDemo.csproj -c Release -- --stress-early-raw

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>

kerryjiang · 2026-05-26T05:19:23Z

+                    var advanced = false;
+
+                    if (cancellationToken.IsCancellationRequested)
+                    {


need comment to explain why it is required

Agreed. I will add a comment here to explain why this is required: when cancellation is observed after ReadAsync has already returned buffered data, we intentionally avoid advancing past any unread bytes so the remaining buffer can still be preserved for the detach/next owner path instead of being dropped.

kerryjiang · 2026-05-26T05:19:50Z

                        {
+                            if (bufferFilterResult.Consumed > 0)


need comment to explain the if/else here

Agreed. I will add a comment for this branch. The Consumed > 0 case means the parser made real progress for a package, so the reader can advance to that consumed position. The else case handles a package result without consumed bytes, where we should only mark the current buffer as examined and avoid dropping unread data.

kerryjiang · 2026-05-26T05:20:01Z

+                            advanced = true;
                            yield return bufferFilterResult.Package;
+
+                            if (cancellationToken.IsCancellationRequested)


need comment to explain the if/else here

Agreed. I will add a comment here as well. After yielding a package, cancellation may be requested by the consumer, so this check stops further parsing while keeping the already-advanced position consistent with the package that was just delivered.

kerryjiang · 2026-05-26T05:20:28Z

+
+namespace KestrelHttp2DetachDemo;
+
+internal sealed class Http2PipelineFilter : PipelineFilterBase<Http2Frame>


should it be in SuperSocket.Http?

Good point. I kept it in the sample because it is currently only a minimal HTTP/2 frame parser for demonstrating the Kestrel detach handoff scenario, not a reusable/general HTTP implementation yet. If you prefer, I can move it under SuperSocket.Http and wire the sample to consume it from there.

interface95 · 2026-05-26T06:04:33Z

I also reran the same commit (ab59f231e846f314115a33f9196415ad80cab63a) in my fork to verify the matrix after the upstream macOS job failed and Windows was cancelled by fail-fast.

Fork Actions run: https://github.com/interface95/SuperSocket/actions/runs/26434481311

Result: Ubuntu, macOS, and Windows all passed. The upstream macOS failure appears to be flaky (WebSocketBasicTest.TestFalseHandshake) rather than a stable failure from this PR.

…es on cancel Address review feedback on PR kerryjiang#831: explain the cancel-before-parse exit, the Consumed>0/==0 split when yielding a package, and the cancel-after-yield re-check that drives loop exit while keeping AdvanceTo consistent.

interface95 · 2026-05-26T07:09:32Z

Thanks for the review. Pushed 0c368a9:

Added inline comments at the three PipeConnectionBase.cs hotspots (cancel-before-parse, Consumed > 0 / == 0 split when yielding a package, cancel-after-yield re-check).
Kept Http2PipelineFilter in the sample for now — it's a minimal HTTP/2 frame parser just to exercise the detach handoff path, not a full HTTP/2 implementation. Happy to promote it into SuperSocket.Http in a follow-up PR if you'd prefer the library to ship a real HTTP/2 pipeline filter.

kerryjiang · 2026-05-29T06:08:51Z

The unit test failed:
https://github.com/kerryjiang/SuperSocket/actions/runs/26410793017

kerryjiang · 2026-05-29T06:13:35Z

@@ -431,12 +469,12 @@ protected async IAsyncEnumerable<TPackageInfo> ReadPipeAsync<TPackageInfo>(PipeR

                    pipelineFilter = _pipelineFilter as IPipelineFilter<TPackageInfo>;



You can put this piece of logic in a bigger if scope

if (!advanced)
{
xxx;
}

Done — consolidated with the change above. The old if (!advanced) block is now absorbed into the single AdvanceTo call site: when advanced == false, examined is set to buffer.End, which is the same behavior as before. No separate scope needed anymore.

See commit ce9c073.

kerryjiang · 2026-05-29T06:25:31Z

+                            // Consumed == 0: the filter produced a package without reporting
+                            // consumption (e.g. it kept its own state); mark the whole buffer
+                            // as examined but only Start as consumed so no bytes are dropped.
+                            if (bufferFilterResult.Consumed > 0)


If you want o advance for each package, you can move this piece of code out from the if statement and then you can remove the advance logic for lastFilterResult which is outside of the foreach.

Done. Moved the AdvanceTo out of the per-package if block and merged it with the post-loop advance into a single call site. The advanced flag now selects the examined position:

advanced == true: examined = consumed (preserve unread bytes for next iteration / detach handoff)

advanced == false: examined = buffer.End (mark entire buffer as examined)

See commit ce9c073.

kerryjiang · 2026-05-29T06:29:35Z

@interface95 left more comments

interface95 · 2026-05-29T08:32:25Z

The only failure is WebSocketBasicTest.TestFalseHandshake on macOS — a known flaky test unrelated to this PR's changes in PipeConnectionBase.cs. I verified all three platforms pass on my fork: https://github.com/interface95/SuperSocket/actions/runs/26434481311

…ance point Move reader.AdvanceTo() out of the per-package if block and merge it with the post-loop advance, using the 'advanced' flag to select the examined position: advanced == true → examined = consumed (preserve unread bytes) advanced == false → examined = buffer.End (mark all as examined) This addresses the review feedback on PR kerryjiang#831 (comments 3322484623, 3322528441) without changing runtime behavior.

claude and others added 3 commits May 26, 2026 00:39

preserve unread data across detach

125f412

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>

add Kestrel HTTP2 detach demo

3f2b25e

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>

include detach demo in samples solution

ab59f23

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>

interface95 mentioned this pull request May 25, 2026

preserve unread data across detach interface95/SuperSocket#1

Closed

kerryjiang reviewed May 26, 2026

View reviewed changes

interface95 mentioned this pull request May 26, 2026

CI run for upstream PR 831 interface95/SuperSocket#2

Closed

kerryjiang reviewed May 29, 2026

View reviewed changes


		namespace KestrelHttp2DetachDemo;

		internal sealed class Http2PipelineFilter : PipelineFilterBase<Http2Frame>

		@@ -431,12 +469,12 @@ protected async IAsyncEnumerable<TPackageInfo> ReadPipeAsync<TPackageInfo>(PipeR

		pipelineFilter = _pipelineFilter as IPipelineFilter<TPackageInfo>;

Conversation

interface95 commented May 25, 2026

Summary

Verification

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

interface95 commented May 26, 2026

Uh oh!

interface95 commented May 26, 2026

Uh oh!

kerryjiang commented May 29, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kerryjiang commented May 29, 2026

Uh oh!

interface95 commented May 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants