Skip to content
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

Wake the task up when inserting a new response into PendingResponses #1735

Closed
wants to merge 3 commits into from

Conversation

dmitry-markin
Copy link
Contributor

This is a hot fix for #1650. Basically, StreamMap suffers from the same footgun as FuturesUnordered: it must be explicitly polled in order for a newly added stream to be registered in the task. The streams are just pushed into the internal container upon insertion: https://github.com/tokio-rs/tokio/blob/master/tokio-stream/src/stream_map.rs#L445-L453

This PR adds a Waker to PendingResponses to make sure the task is woken up when a new response is added.

@dmitry-markin dmitry-markin added R0-silent Changes should not be mentioned in any release notes T0-node This PR/Issue is related to the topic “node”. labels Sep 28, 2023
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3827638

Copy link
Contributor

@altonen altonen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in general it would be good to test all non-trivial networking changes on a live node since the test coverage of sc-network has proven to be insufficient

@@ -79,6 +80,10 @@ impl<B: BlockT> PendingResponses<B> {
);
debug_assert!(false);
}

if let Some(waker) = self.waker.take() {
waker.wake();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This requires that we called poll before. Not sure that this is removing any footgun.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the code we are already polling pending_responses after we inserted any kind of element, so why is the task not waking up?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This requires that we called poll before. Not sure that this is removing any footgun.

Yeah, but we definitely called it at least once.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the code we are already polling pending_responses after we inserted any kind of element, so why is the task not waking up?

There is no problem with waking up right now, but it can shoot us once the polling in SyncingEngine is converted into select! and the order of inserting / polling pending_responses is not strictly defined.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But for a select!. Once you insert data to the pending_responses your match arm will be finished and you start again from the top of the loop, aka calling select! again. This will poll all futures, including your pending_responses futures. So, not sure why we could miss there to poll it.

serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Mar 26, 2024
* Handle SIGTERM for some docker containers

* Implement SIGTERM handling for the relay
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Mar 27, 2024
* Handle SIGTERM for some docker containers

* Implement SIGTERM handling for the relay
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
* Handle SIGTERM for some docker containers

* Implement SIGTERM handling for the relay
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
* Handle SIGTERM for some docker containers

* Implement SIGTERM handling for the relay
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
* Handle SIGTERM for some docker containers

* Implement SIGTERM handling for the relay
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
* Handle SIGTERM for some docker containers

* Implement SIGTERM handling for the relay
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
* Handle SIGTERM for some docker containers

* Implement SIGTERM handling for the relay
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
* Handle SIGTERM for some docker containers

* Implement SIGTERM handling for the relay
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
* Handle SIGTERM for some docker containers

* Implement SIGTERM handling for the relay
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
* Handle SIGTERM for some docker containers

* Implement SIGTERM handling for the relay
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
* Handle SIGTERM for some docker containers

* Implement SIGTERM handling for the relay
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
* Handle SIGTERM for some docker containers

* Implement SIGTERM handling for the relay
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
* Handle SIGTERM for some docker containers

* Implement SIGTERM handling for the relay
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 10, 2024
* Handle SIGTERM for some docker containers

* Implement SIGTERM handling for the relay
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 10, 2024
* Handle SIGTERM for some docker containers

* Implement SIGTERM handling for the relay
bkchr pushed a commit that referenced this pull request Apr 10, 2024
* Handle SIGTERM for some docker containers

* Implement SIGTERM handling for the relay
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes T0-node This PR/Issue is related to the topic “node”.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants