Don't wait for push before sending headers
#119
Closed
Conversation
This file contains 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
This paves the way for abstracting this case into a separate function in the next commit.
See comments in the code itself for justification.
Prior to this refactoring, it was necessary to delay enqueuing a new `Output` until there was data available: without that delay, `outputOrEnqueueAgain` would check the queue and the streaming window, realize that the queue was empty, and then re-enqueue the `Output`, causing it to be handled out-of-order (which is problematic, because streams must be opened in order). We can't simply change `outputOrEnqueueAgain` _not_ to check the queue, because (_after_ we have sent the original headers), if there are no output available, we should indeed output other streams first. However, in the previous steps we changed `outputOrEnqueueAgain` to treat the stream initialization (`OObj`) case special: don't check the queue nor the window size, but send the headers immediately, and only _then_ check the rest. This means that we no longer have to wait: when the stream is initialized, the headers will be sent immediately, thereby establishing the stream, in the appropriate order. The benefit to the _user_ is that the request is now initialized before any data is sent, which may be important for some applications.
kazu-yamamoto
approved these changes
Jun 7, 2024
|
Rebased and merged. |
|
Thanks @kazu-yamamoto , I appreciate that :) The next PR is coming up soon. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
In a76cdf3 a change was introduced which meant that a streaming request was not enqueued until at least some data was available. The reason (I think) was that without that delay,
outputOrEnqueueAgainwould check the queue and the streaming window, realize that the queue was empty, and then re-enqueue theOutput, causing it to be handled out-of-order (which is problematic, because streams must be opened in order).We can't simply change
outputOrEnqueueAgainnot to check the queue, because if there are no output available, we should indeed process other streams first. However, we can add a special case tooutputOrEnqueueAgainso that it doesn't check either whether data is available or the streaming window in the case of anOObjobject (which will initialize the stream by sending the request headers). This is sound, because the headers do not depend on the initial data (they are fully known at this point), and the streaming window only applies toDATAframes.Once we do this, we can enqueue the request before any data is available, which can be important for some applications where the request headers must be sent even if no data is available. This PR does this in 6 steps: steps 1-4 don't change the semantics at all, but do some step-by-step refactoring to make the next two changes easier to follow; step 5 makes the change to
outputOrEnqueueAgain, and step 6 finally changessendStreamingnot to wait for data to become available.Side note: I was previously working around this by calling
pushwith an empty string, but this causes problems of its own. Indeed, I hope to be following up to this PR with a second PR that makes further changes; that second PR does perhaps not rely on this one, but this PR does make that second PR much easier to verify, I think. That second PR is not yet ready, but submitting this one already in case I'm doing something here that is completely broken.