Fix notifications sometimes not being sent #7594

tomaka · 2020-11-24T19:30:08Z

In the poll function of handler.rs, we first call Sink::poll_flush on all the outbound substreams, then only call Sink::start_send. This means that the actual flushing is only done the next time poll is called, which is in no way guaranteed to happen soon.

While this little fix might seem like a hack, and it would be preferable to change the order of operations in poll, doing so is a bit tricky to me because of having to handle state transitions when the substream errors. Since this code is undergoing another rewrite in #7374, I went for the easy solution.

mxinden

While this little fix might seem like a hack, and it would be preferable to change the order of operations in poll, doing so is a bit tricky to me because of having to handle state transitions when the substream errors. Since this code is undergoing another rewrite in #7374, I went for the easy solution.

I am fine with this change as long as it is removed with #7374.

mxinden · 2020-11-24T19:53:39Z

client/network/src/protocol/generic_proto/handler.rs

 						if let Some(pos) = self.out_protocols.iter().position(|(n, _)| *n == protocol_name) {
 							if let Some(substream) = out_substreams[pos].as_mut() {
 								let _ = substream.start_send_unpin(message);
+								cx.waker().wake_by_ref();


Would you mind documenting that this wake up is used to force another call to NotifsHandler::poll and thus to flush the corresponding substreams?

In addition I would suggest documenting that this is temporary and removed with #7374.

wheresaddie

I approve this based on it solving/removing #7374

mxinden · 2020-11-25T08:57:19Z

I approve this based on it solving/removing #7374

@wheresaddie to make sure there is no confusion: This pull request does not make #7374 obsolete. Quite the opposite as #7374 would remove the hack introduced in this pull request.

tomaka · 2020-11-25T09:15:34Z

bot merge

ghost · 2020-11-25T09:15:37Z

Trying merge.

* CI: build docs after test; publish docs after build (paritytech#7591) docs time test/build success on master pub * node-template: add aura to light block import pipeline (paritytech#7595) added aura to block import pipeline * Fix notifications sometimes not being sent (paritytech#7594) * Fix notifications sometimes not being sent * Add comment * Bump rpassword from 4.0.5 to 5.0.0 (paritytech#7597) Bumps [rpassword](https://github.com/conradkleinespel/rpassword) from 4.0.5 to 5.0.0. - [Release notes](https://github.com/conradkleinespel/rpassword/releases) - [Commits](conradkleinespel/rpassword@v4.0.5...v5.0.0) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * remove std feature flags for assert macros (paritytech#7600) * remove std feature flags for assert macros * re-add note about availability in no_std envs * Add small header cache (paritytech#7516) * Remove header query * Header cache * Fix potential race issue * Simplify status query * Inform sync explicitly about new best block (paritytech#7604) * Inform sync explicitly about new best block Instead of "fishing" the new best block out of the processed blocks, we now tell sync directly that there is a new best block. It also makes sure that we update the corresponding sync handshake to the new best block. This is required for parachains as they first import blocks and declare the new best block after being made aware of it by the relay chain. * Adds test * Make sure async stuff had time to run * Bump directories from 2.0.2 to 3.0.1 (paritytech#7609) Bumps [directories](https://github.com/soc/directories-rs) from 2.0.2 to 3.0.1. - [Release notes](https://github.com/soc/directories-rs/releases) - [Commits](https://github.com/soc/directories-rs/commits) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Remove `RpcMetrics` weirdness (paritytech#7608) * Remove `RpcMetrics` weirdness The metrics was returning an error when prometheus was not given. This was a really weird setup, especially when compared to all other metrics that just do nothing if there is no registry. * Fix browser build * Upgrade to libp2p-0.31. (paritytech#7606) * Upgrade to libp2p-0.31. * Address line width. * Add generous incoming connection limit. * Remove old noise configuration. * Add Key Subcommand to node-template (paritytech#7615) * Forward storage changes in manual seal (paritytech#7614) This prevents nodes from executing the same block 2 times. * chore/error: remove from str conversion and add deprecation notificat… (paritytech#7472) * chore/error: remove from str conversion and add deprecation notifications * fixup changes * fix test looking for gone ::Msg variant * another test fix * one is duplicate, the other is not, so duplicates reported are n-1 * darn spaces Co-authored-by: Andronik Ordian <[email protected]> * remove pointless doc comments of error variants without any value * low hanging fruits (for a tall person) * moar error type variants * avoid the storage modules for now They are in need of a refactor, and the pain is rather large removing all String error and DefaultError occurences. * chore remove pointless error generic * fix test for mocks, add a bunch of non_exhaustive * max line width * test fixes due to error changes * fin * error outputs... again * undo stderr adjustments * Update client/consensus/slots/src/lib.rs Co-authored-by: Bastian Köcher <[email protected]> * remove closure clutter Co-authored-by: Bastian Köcher <[email protected]> * more error types * introduce ApiError * extract Mock error * ApiError refactor * even more error types * the last for now * chore unused deps * another extraction * reduce should panic, due to extended error messages * error test happiness * shift error lines by one * doc tests * white space Co-authored-by: Bastian Köcher <[email protected]> * Into -> From Co-authored-by: Bastian Köcher <[email protected]> * remove pointless codec Co-authored-by: Bastian Köcher <[email protected]> * avoid pointless self import Co-authored-by: Bastian Köcher <[email protected]> Co-authored-by: Bernhard Schuster <[email protected]> Co-authored-by: Andronik Ordian <[email protected]> Co-authored-by: Bastian Köcher <[email protected]> * network: don't force send block announcements (paritytech#7601) * Change TRACING_SET to static (paritytech#7607) * change TRACING_SET to static * Update primitives/io/src/lib.rs Co-authored-by: Bastian Köcher <[email protected]> * modify test with nested spans Co-authored-by: Bastian Köcher <[email protected]> * sc-network: Log outgoing notifications too (paritytech#7624) * Log outgoing notifications too * Update client/network/src/protocol/generic_proto/handler.rs Co-authored-by: Max Inden <[email protected]> Co-authored-by: Addie Wagenknecht <[email protected]> Co-authored-by: Max Inden <[email protected]> * Bump console_log from 0.1.2 to 0.2.0 (paritytech#7623) Bumps [console_log](https://github.com/iamcodemaker/console_log) from 0.1.2 to 0.2.0. - [Release notes](https://github.com/iamcodemaker/console_log/releases) - [Commits](https://github.com/iamcodemaker/console_log/commits) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * `sudo_as` should return a result (paritytech#7620) * Fix wrong value put for pending_opening (paritytech#7633) * Fix wrong value put for pending_opening * Oops, didn't even try compiling it * Rename pallet trait `Trait` to `Config` (paritytech#7599) * rename Trait to Config * add test asserting using Trait is still valid. * fix ui tests * resolve unresolved error nits of paritytech#7617 (paritytech#7631) * handle executor should_panic test better * Revert "reduce should panic, due to extended error messages" This reverts commit c080594. * remove excessive constraints * remove duplicate documentation messages for error variants * reduce T: constraints to the abs minimum * whoops * fewer bounds again Co-authored-by: Bernhard Schuster <[email protected]> * Fix bad state transition with DisabledPendingEnable+OpenDesiredByRemote (paritytech#7638) * Renames of `Trait` to `Config` in README.md, weight templates and few minor ones (paritytech#7636) * manual rename * renamse in README.md * fix template * Fix CI Link Check (paritytech#7639) * fix trigger fingers * more * Update frame/example-offchain-worker/README.md Co-authored-by: Guillaume Thiolliere <[email protected]> Co-authored-by: Bastian Köcher <[email protected]> Co-authored-by: Guillaume Thiolliere <[email protected]> * Fix cargo clippy warning in peerset. (paritytech#7641) * Fix cargo clippy warning in peerset. * Update client/peerset/src/lib.rs Co-authored-by: Bastian Köcher <[email protected]> * Apply suggestions from code review Co-authored-by: Pierre Krieger <[email protected]> Co-authored-by: Bastian Köcher <[email protected]> Co-authored-by: Pierre Krieger <[email protected]> Co-authored-by: Denis Pisarev <[email protected]> Co-authored-by: André Silva <[email protected]> Co-authored-by: Pierre Krieger <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Alexander Popiak <[email protected]> Co-authored-by: Arkadiy Paronyan <[email protected]> Co-authored-by: Bastian Köcher <[email protected]> Co-authored-by: Roman Borschel <[email protected]> Co-authored-by: Benjamin Kampmann <[email protected]> Co-authored-by: Bernhard Schuster <[email protected]> Co-authored-by: Bernhard Schuster <[email protected]> Co-authored-by: Andronik Ordian <[email protected]> Co-authored-by: Andrew Plaza <[email protected]> Co-authored-by: Addie Wagenknecht <[email protected]> Co-authored-by: Max Inden <[email protected]> Co-authored-by: Shawn Tabrizi <[email protected]> Co-authored-by: Guillaume Thiolliere <[email protected]> Co-authored-by: jolestar <[email protected]>

* Fix notifications sometimes not being sent * Add comment

Fix notifications sometimes not being sent

fb1e3cd

tomaka added A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Nov 24, 2020

tomaka requested review from mxinden and romanb November 24, 2020 19:30

mxinden approved these changes Nov 24, 2020

View reviewed changes

Add comment

ab31856

wheresaddie approved these changes Nov 25, 2020

View reviewed changes

romanb approved these changes Nov 25, 2020

View reviewed changes

ghost merged commit bce646d into paritytech:master Nov 25, 2020

tomaka deleted the fix-the-world branch November 25, 2020 09:15

darkfriend77 pushed a commit to mogwaicoin/substrate that referenced this pull request Jan 11, 2021

Fix notifications sometimes not being sent (paritytech#7594)

571cb75

* Fix notifications sometimes not being sent * Add comment

This was referenced Mar 21, 2021

Distribute back pressure evenly in notification protocols. #8410

Closed

Refactor NotifsHandler::poll #8422

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix notifications sometimes not being sent #7594

Fix notifications sometimes not being sent #7594

Uh oh!

tomaka commented Nov 24, 2020 •

edited

Loading

Uh oh!

mxinden left a comment

Uh oh!

mxinden Nov 24, 2020

Uh oh!

wheresaddie left a comment

Uh oh!

mxinden commented Nov 25, 2020

Uh oh!

tomaka commented Nov 25, 2020

Uh oh!

ghost commented Nov 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Fix notifications sometimes not being sent #7594

Fix notifications sometimes not being sent #7594

Uh oh!

Conversation

tomaka commented Nov 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mxinden left a comment

Choose a reason for hiding this comment

Uh oh!

mxinden Nov 24, 2020

Choose a reason for hiding this comment

Uh oh!

wheresaddie left a comment

Choose a reason for hiding this comment

Uh oh!

mxinden commented Nov 25, 2020

Uh oh!

tomaka commented Nov 25, 2020

Uh oh!

ghost commented Nov 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tomaka commented Nov 24, 2020 •

edited

Loading