Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Fix notifications sometimes not being sent #7594

Merged
2 commits merged into from
Nov 25, 2020
Merged

Fix notifications sometimes not being sent #7594

2 commits merged into from
Nov 25, 2020

Conversation

tomaka
Copy link
Contributor

@tomaka tomaka commented Nov 24, 2020 •
edited
Loading

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.

@tomaka 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
mxinden
mxinden approved these changes Nov 24, 2020
View reviewed changes
Copy link
Contributor

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

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.

client/network/src/protocol/generic_proto/handler.rs
@@ -971,6 +971,7 @@ impl ProtocolsHandler for NotifsHandler {
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();
Copy link
Contributor

Choose a reason for hiding this comment

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

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
wheresaddie approved these changes Nov 25, 2020
View reviewed changes
Copy link
Contributor

@wheresaddie wheresaddie left a comment

Choose a reason for hiding this comment

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

I approve this based on it solving/removing #7374

@mxinden
Copy link
Contributor

mxinden commented Nov 25, 2020

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
Copy link
Contributor Author

tomaka commented Nov 25, 2020

bot merge

@ghost
Copy link

ghost commented Nov 25, 2020

Trying merge.

@ghost ghost merged commit bce646d into paritytech:master Nov 25, 2020
@tomaka tomaka deleted the fix-the-world branch November 25, 2020 09:15
clearloop added a commit to patractlabs/substrate that referenced this pull request Dec 1, 2020
@clearloop @TriplEight
@andresilva @tomaka @dependabot @apopiak @arkpar @bkchr @romanb @gnunicorn @drahnr @ordian @insipx @wheresaddie @mxinden @shawntabrizi @gui1117 @jolestar
Sync upstream on 2020-12-01 (#10)
4bcbc12 
* 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] <support@github.com>

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] <support@github.com>

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 <write@reusable.software>

* 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 <bkchr@users.noreply.github.com>

* remove closure clutter

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

* 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 <bkchr@users.noreply.github.com>

* Into -> From

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

* remove pointless codec

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

* avoid pointless self import

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

Co-authored-by: Bernhard Schuster <bernhard@parity.io>
Co-authored-by: Andronik Ordian <write@reusable.software>
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

* 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 <bkchr@users.noreply.github.com>

* modify test with nested spans

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

* 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 <mail@max-inden.de>

Co-authored-by: Addie Wagenknecht <addie@nortd.com>
Co-authored-by: Max Inden <mail@max-inden.de>

* 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] <support@github.com>

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 <bernhard@parity.io>

* 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 <gui.thiolliere@gmail.com>

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>

* 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 <bkchr@users.noreply.github.com>

* Apply suggestions from code review

Co-authored-by: Pierre Krieger <pierre.krieger1708@gmail.com>

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
Co-authored-by: Pierre Krieger <pierre.krieger1708@gmail.com>

Co-authored-by: Denis Pisarev <denis.pisarev@parity.io>
Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com>
Co-authored-by: Pierre Krieger <pierre.krieger1708@gmail.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Alexander Popiak <alexander.popiak@parity.io>
Co-authored-by: Arkadiy Paronyan <arkady.paronyan@gmail.com>
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
Co-authored-by: Roman Borschel <romanb@users.noreply.github.com>
Co-authored-by: Benjamin Kampmann <ben@gnunicorn.org>
Co-authored-by: Bernhard Schuster <bernhard@ahoi.io>
Co-authored-by: Bernhard Schuster <bernhard@parity.io>
Co-authored-by: Andronik Ordian <write@reusable.software>
Co-authored-by: Andrew Plaza <aplaza@liquidthink.net>
Co-authored-by: Addie Wagenknecht <addie@nortd.com>
Co-authored-by: Max Inden <mail@max-inden.de>
Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
Co-authored-by: jolestar <jolestar@gmail.com>
darkfriend77 pushed a commit to mogwaicoin/substrate that referenced this pull request Jan 11, 2021
@tomaka @darkfriend77
Fix notifications sometimes not being sent (paritytech#7594)
571cb75 
* Fix notifications sometimes not being sent

* Add comment
This was referenced Mar 21, 2021
Closed
Merged
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@romanb romanb romanb approved these changes

@wheresaddie wheresaddie wheresaddie approved these changes

@mxinden mxinden mxinden approved these changes

Assignees
No one assigned
Labels
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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tomaka @mxinden @romanb @wheresaddie