-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix(swarm): avoid re-extracting completed outbound substreams #6427
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
base: master
Are you sure you want to change the base?
Changes from 2 commits
9443583
4b11ee2
e563501
f43eff8
120508d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -411,7 +411,15 @@ where | |
| } | ||
| } | ||
|
|
||
| if let Some(requested_substream) = requested_substreams.iter_mut().next() { | ||
| // Skip `Done` entries: `extract()` leaves them behind until the | ||
| // entry's `Future::poll` removes them on the next `poll_next`. | ||
| // If `extract()` runs before that future has stored its waker, | ||
| // the `Done` entry can persist and a plain `iter_mut().next()` | ||
| // would re-extract it and panic with "cannot extract twice". | ||
| if let Some(requested_substream) = requested_substreams | ||
| .iter_mut() | ||
| .find(|r| matches!(r, SubstreamRequested::Waiting { .. })) | ||
| { | ||
| match muxing.poll_outbound_unpin(cx)? { | ||
| Poll::Pending => {} | ||
| Poll::Ready(substream) => { | ||
|
|
@@ -855,6 +863,36 @@ mod tests { | |
| )) | ||
| } | ||
|
|
||
| // Regression test for `panic!("cannot extract twice")`: pushing two | ||
| // entries and calling `extract()` without ever polling them leaves | ||
| // a `Done` entry that `iter_mut().next()` would re-extract. The | ||
| // `find(Waiting)` filter must skip past it. | ||
| #[test] | ||
| fn iter_mut_skips_done_substream_requested_entries() { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this test manually creates the race condition scenario by directly manipulating the internal state, it doesn't actually prove the panic can happen in real-world usage.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. I replaced the direct |
||
| let mut requested: FuturesUnordered<SubstreamRequested<(), DeniedUpgrade>> = | ||
| FuturesUnordered::new(); | ||
| for _ in 0..2 { | ||
| requested.push(SubstreamRequested::new( | ||
| (), | ||
| Duration::from_secs(60), | ||
| DeniedUpgrade, | ||
| )); | ||
| } | ||
|
|
||
| for _ in 0..2 { | ||
| let _ = requested | ||
| .iter_mut() | ||
| .find(|r| matches!(r, SubstreamRequested::Waiting { .. })) | ||
| .unwrap() | ||
| .extract(); | ||
| } | ||
| assert!( | ||
| requested | ||
| .iter_mut() | ||
| .all(|r| matches!(r, SubstreamRequested::Done)) | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn propagates_changes_to_supported_inbound_protocols() { | ||
| let mut connection = Connection::new( | ||
|
|
||
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.
we could just make
extractreturn anOption.We could then
finding here for aSubstreamRequested::Waitingto then match again insideextractfeels redundant.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.
Done.
extract()now returnsOption, and the connection path usesfind_map(SubstreamRequested::extract)to skip staleDoneentries instead of panicking. I kept a separate waiting-request precheck before polling the muxer so we do not open an outbound stream unless there is a request to pair with it.