feat: reintroduce DuplexStream
#327
Merged
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
nyannyacha
changed the title
DuplexStream
nyannyacha
force-pushed
the
fix-conn-lt
branch
3 times, most recently
from
9aea940
to
6bd3efc
Compare
laktek
approved these changes
May 7, 2024
|
🎉 This PR is included in version 1.48.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
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.
What kind of change does this PR introduce?
Bug fix, Enhancement
Description
Major changes compared by PR-320
Makes the timing of pipe closure is adjustable within a separately created tokio task, so that the stream is not closed in the middle of a response.
In the case of
DuplexStream, if either pair is dropped, the stream immediately goes into an unwritable state.These streams must remain writable for outside responses, but the previous PR did not. So this issue was immediately exposed by the flush behavior of libraries like
hyper.With the current implementation, which requires a Tokio task to be created every time a stream is dropped, I get a small performance hit compared to PR-320, but I believe we can replace it with an efficient implementation in the next iteration.
Notes on Minor changes
ConnSynchas been removed and replaced withCancellationToken.This was a refactoring to simplify the structure, and while the performance impact of the replacement has not been carefully evaluated, it does not appear to have changed much.
Stream of request body must be cancelable from errors or exceptions by the worker.
Sometimes the request body is not fully consumed by the worker due to circumstances such as worker resource limit due to a multipart request.
Previously,
sb_workersdid not handle these circumstances properly, leaving the end requestor in a situation where they would receive no response indefinitely. (Runtime also had resource leaks due to unresolved promises)This PR introduces a routine that tries to cancel the request body stream after checking for a response or exception from the worker.
Graceful shutdown by
SIGWINCHis now behind thebase/termination-signal-extfeature flag.Given the nature of devs frequently changing the terminal size, SIGWINCH has been gated to only be enabled behind a specific feature flag.