Stream receives report the end of the stream - #113
Open
rnro wants to merge 3 commits into
Open
Conversation
The property reported true if *any* frame carried `connectionComplete`, which makes it a question about the array's contents rather than about the stream. A stream ends after its last byte, so a flag on a frame with bytes behind it does not describe a stream that has ended, and reading it that way reports the end early. This matches QUIC and TCP, which both put the end-of-stream marker on the last byte and allow nothing after it. Nothing was relying on the any-frame reading; there was simply nothing stating where the flag may sit. The only place that marks an inbound frame already puts it on the last one. Reading the final frame states the rule, and makes a stray or stale mark irrelevant rather than harmful, so no writer has to be policed. `markEndOfStream` no longer needs to explain itself, and `ProtocolStreamHandlers.receiveStreamData`, which gates on the array flag to release data below the caller's minimum, can no longer be tricked into releasing early.
With the array reporting the flag from its final frame, whatever writes the flag has to put it there, and a byte-limited drain has to keep it there. `markEndOfStream()` marks the final frame; the socket bottom previously marked every queued frame, which reported the end of the stream while bytes were still queued. `drainArray` moves the flag to the tail it retains when it splits a frame: one branch swaps the original out as the returned prefix, and since `swap` exchanges whole frame values the flag would otherwise leave with the prefix and be lost from the bytes that remain. Which frame carries the flag is an invariant of the container rather than of whatever fills it, and it is the invariant `drainArray` already relies on, so both belong here.
At the moment `Message.isComplete` is always false for a stream receive, whatever the sender did: the flag travels on a frame as `connectionComplete` and the socket bottom sets it, but the stream read path accumulated only the bytes and dropped the flag before any consumer could see it. That leaves a consumer inferring half-closure from a receive completing with no content, which is a different mechanism and an unreliable one. It works only when the FIN arrives in a read event of its own; when it arrives alongside data, `drainArray` hands the sentinel over with the payload, so there is no empty receive either and the next one never completes. Nothing in the library reads the flag, which is why no test caught it. `read(minimumBytes:maximumBytes:)` now returns whether the stream ended alongside the content, and the socket bottom marks the queued bytes rather than appending a sentinel. That means no frame has to be allocated at EOF, and keying the decision on the half-closed state rather than on a queued frame is what lets the marker be synthesized once the queue has already drained. Delivery is recorded from the drained result as well, so the common path where the flag rides out on real data does not then report a second, empty end of stream.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
At the moment
Message.isCompleteis always false for a stream receive, whatever the sender did. The flag travels on a frame asconnectionCompleteand the socket bottom sets it, but the stream read path accumulated only the bytes and dropped the flag before any consumer could see it. That leaves a consumer inferring half-closure from a receive completing with no content, which is a different mechanism and an unreliable one. It works only when the FIN arrives in a read event of its own; when it arrives alongside datadrainArrayhands the sentinel over with the payload, so there is no empty receive either and the next one never completes.Three commits, in this order:
FrameArray.connectionCompletenow reads the final frame, where it previously reported true if any frame carried the flag. This is a semantics change to a public property. I think this correctly matches the intended semantics though and matches my expectations from common protocols e.g. QUIC and TCP which both put the marker on the last byte and allow nothing after it. It also closes a latent premature release inProtocolStreamHandlers.receiveStreamData, which gates on the array flag to hand over data below the caller's minimum.drainArraykeeps the flag with the tail it retains when it splits. Previously the flag went the wrong way.read(minimumBytes:maximumBytes:)andEndpointFlowcarry it up toMessage.isComplete.