-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: avoid race condition between pending frames and closing stream #156
Merged
Merged
Changes from 10 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
dce9ea5
Add failing unit test for missing flush
thomaseizinger 76a7e6f
Actually wait for flushing on `yamux::Stream`
thomaseizinger 6c03282
Flushing a closed stream is okay
thomaseizinger 496613e
Use 1 channel per stream
thomaseizinger ee3898a
Minimize diff
thomaseizinger 53d90e4
Introduce dedicated `CommandReceivers`
thomaseizinger 09a660e
Fix compile error in unit test
thomaseizinger 4b5293b
Fix a bug where closing would hang forever
thomaseizinger c6acd8e
Replace `garbage_collect` with detecting closed receiver
thomaseizinger e0ec0aa
Correctly poll all receivers
thomaseizinger e755c2c
Introduce `TaggedStream` so we can use `SelectAll`
thomaseizinger fd810c4
Inline `poll_stream_receivers` and remove ID from stream command
thomaseizinger 4060d07
Add comment about channel size
thomaseizinger e809103
Update docs
thomaseizinger 53d5ece
Reduce diff
thomaseizinger File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,4 +16,3 @@ log = "0.4.17" | |
[dev-dependencies] | ||
env_logger = "0.10" | ||
constrained-connection = "0.1" | ||
|
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 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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Polling each stream
Receiver
even though only one might bePoll::Ready
seems wasteful. WouldSelectAll
not be an option? One would wrap eachReceiver
such that it returns theStreamId
when theReceiver
returnsPoll::Ready(None)
. With thatStreamId
we can then callself.on_drop_stream(id)
.(On consecutive polls the wrapper can return
Poll::Ready(None)
and thus it would be cleaned up by theSelectAll
.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a
SelectAll
based design before but dropped it because I couldn't detectNone
from the outside.Detecting that requires another data structure for translating between the stream commands and the actual frames in connection (the wrapping you mentioned).
This also needs to work for the
Cleanup
andClose
case.I tried hiding it in a custom collection object but we need access to the stream
IntMap
upon drop so that needs even more refactoring but would be a clean design.I can invest the time if you want but I don't think it is a quick 30min refactoring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wrong, it was actually a super quick refactoring, now that I've seen how tokio's
StreamMap
works. They recommend an additional wrapper aroundStream
. The trick is to wrap the output of theStream
again such that you get a tuple of(key, Option<Item>)
which allows you to detect closing of the stream.