Generic request/response infrastructure for Polkadot #2352

eskimor · 2021-01-29T18:55:36Z

Generic infrastructure for sending and receiving of requests and responses from subsystems.

This PR depends on this substrate PR, which just got merged - so this should be good to go.

node/network/availability-distribution/src/lib.rs

node/network/protocol/src/request_response/v1.rs

node/network/protocol/src/request_response.rs

node/network/bridge/src/multiplexer.rs

node/network/availability-distribution/src/lib.rs

node/network/protocol/src/request_response.rs

node/network/bridge/src/multiplexer.rs

It is not protocol related at all, it is in fact only part of the subsystem communication as it gets wrapped into messages of each subsystem.

WIP: Does not compile.

Request multiplexer is moved to bridge, as there the implementation is more straight forward as we can specialize on `AllMessages` for the multiplexing target. Sending of requests is mostly complete, apart from a few `From` instances. Receiving is also almost done, initializtion needs to be fixed and the multiplexer needs to be invoked.

Subsystems are now able to receive and send requests and responses via the overseer.

- start encoding at 0 - don't crash on zero protocols - don't panic on not yet implemented request handling

Use index 0 instead of 1. Co-authored-by: Andronik Ordian <write@reusable.software>

node/network/bridge/src/lib.rs

node/network/bridge/src/multiplexer.rs

ordian · 2021-02-03T12:22:20Z

node/network/bridge/src/multiplexer.rs

+				// Receiver is a fused stream, which allows for this simple handling of 
+				// exhausted ones.
+				Poll::Ready(None) => {}


Is that expected that one of receivers streams can end?
If not, maybe we should abort in this case by returning Poll::Ready(None) instead of silently ignoring it?
Aso, is it safe to poll it if is_terminated is true? Not sure.

Receiver is a fused Stream, so it should be safe. Not sure about the first question. It seemed logical that the stream ends if all its sources are exhausted.

The only reason those streams should get exhausted is because we are shutting down, in that case it might or might not matter to deliver any messages still in the queues. It felt a bit safer to try to deliver them, until everything is exhausted. On the other hand, receivers will hardly be able to respond anyways ....

I guess that's an edge case, which should not make much of a difference, either way.

Receiver is a fused Stream, so it should be safe.

it still can panic:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=f1ac00093b376fc6cefb41033adff814

oh boy, thanks a lot! I could have sworn that it is safe to poll a fused stream after exhaustion.

I think the safest thing would be to early return on the first closed stream, i.e. return Ready(None).

I think the issue is slightly skewed, if one fuses the inner streams, so they can be polled it will work.

So what you want is: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=691bfc7175278e2887340f5289df63d3

Ok, so I confused a Stream implementing FusedStream with a stream that got fused by means of fuse. Ok this definitely deserves a test. Thank you guys. Will provide a fix in a minute.

node/network/bridge/src/multiplexer.rs

node/network/protocol/src/request_response.rs

node/network/bridge/src/lib.rs

ordian

Thanks!

node/network/bridge/src/multiplexer.rs

eskimor · 2021-02-03T14:24:45Z

node/network/bridge/src/multiplexer.rs

 			let (p, rx): &mut (_, _) = &mut self.receivers[i % len];
 			i += 1;
 			count -= 1;
 			match Pin::new(rx).poll_next(cx) {
 				// If at least one stream is pending, then we are not done yet (No
-                // Ready(None)).
+				// Ready(None)).


At some point, I will get my editor to do this right. It works like 99% of the time, but then suddenly expandtab is activated again for some reason. arrgh

node/network/availability-distribution/src/lib.rs

node/network/bridge/src/action.rs

drahnr · 2021-02-03T14:34:35Z

node/network/bridge/src/lib.rs

 			}
 		};

 		match action {
 			Action::Nop => {}
-			Action::Abort => return Ok(()),
+			Action::Abort(reason) => match reason {
+				AbortReason::SubsystemError(err) => {


If AbortReason becomes an Error type, this could become trivial as well.

Ok, you have a point. I am already bending my definition of error.

Na, I think it is good enough for now :-) I finally want to carry on with availability distribution.

node/network/protocol/src/request_response.rs

node/network/bridge/src/network.rs

node/network/bridge/src/multiplexer.rs

drahnr

A few very small nits, other than that looks good to me 👍

Co-authored-by: Bernhard Schuster <bernhard@ahoi.io>

- Channel size is now determined by function. - Explicitely scope NetworkService::start_request.

…o rk-req-resp-2306

eskimor · 2021-02-03T20:21:04Z

bot merge

ghost · 2021-02-03T20:21:09Z

Trying merge.

* master: Diagnostics quality of life improvements (#2375) Generic request/response infrastructure for Polkadot (#2352) Fix bug in statement table (#2369)

github-actions bot added the A0-please_review Pull request needs code review. label Jan 29, 2021

eskimor added 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 Jan 29, 2021

eskimor requested review from drahnr and ordian January 29, 2021 18:57

drahnr reviewed Jan 29, 2021

View reviewed changes

node/network/availability-distribution/src/lib.rs Outdated Show resolved Hide resolved

ordian reviewed Jan 29, 2021

View reviewed changes

node/network/protocol/src/request_response/v1.rs Outdated Show resolved Hide resolved

ordian reviewed Jan 29, 2021

View reviewed changes

node/network/protocol/src/request_response.rs Outdated Show resolved Hide resolved

node/network/bridge/src/multiplexer.rs Outdated Show resolved Hide resolved

node/network/availability-distribution/src/lib.rs Outdated Show resolved Hide resolved

eskimor requested a review from tomaka January 29, 2021 20:56

ordian reviewed Jan 29, 2021

View reviewed changes

node/network/protocol/src/request_response.rs Outdated Show resolved Hide resolved

ordian reviewed Jan 29, 2021

View reviewed changes

node/network/bridge/src/multiplexer.rs Show resolved Hide resolved

eskimor force-pushed the rk-req-resp-2306 branch from 554f71a to 3b5fb08 Compare February 2, 2021 19:55

eskimor requested review from drahnr and ordian February 2, 2021 19:58

eskimor force-pushed the rk-req-resp-2306 branch from 8e3065f to 68b879a Compare February 2, 2021 21:10

eskimor and others added 15 commits February 3, 2021 09:40

Move NetworkBridgeEvent to subsystem::messages.

f263943

It is not protocol related at all, it is in fact only part of the subsystem communication as it gets wrapped into messages of each subsystem.

Request/response infrastructure is taking shape.

3bb1e37

WIP: Does not compile.

Multiplexer variant not supported by Rusts type system.

0bbd8c1

request_response::request type checks.

b30ba0f

Cleanup.

0d23861

Minor fixes for request_response.

1de47ac

Remove obsolete multiplexer.

4e53e79

Initialize bridge with multiplexer.

857ab9c

Finish generic request sending/receiving.

1204faf

Subsystems are now able to receive and send requests and responses via the overseer.

Doc update.

3ae3171

Fixes.

06f9966

Link issue for not yet implemented code.

a6508ac

Fixes suggested by @ordian - thanks!

5a718dd

- start encoding at 0 - don't crash on zero protocols - don't panic on not yet implemented request handling

Update node/network/protocol/src/request_response/v1.rs

d73b567

Use index 0 instead of 1. Co-authored-by: Andronik Ordian <write@reusable.software>

Fix tests.

10320ce

ordian reviewed Feb 3, 2021

View reviewed changes

eskimor mentioned this pull request Feb 3, 2021

Determine good upper bounds for request/response sizes #2370

Closed

eskimor added 4 commits February 3, 2021 14:24

Add link to issue.

e0c8545

Clarify comments some more.

bc76e28

Fix tests.

553b1e8

Formatting fix.

7600e96

ordian approved these changes Feb 3, 2021

View reviewed changes

node/network/bridge/src/multiplexer.rs Outdated Show resolved Hide resolved

node/network/bridge/src/multiplexer.rs Outdated Show resolved Hide resolved

tabs

468ce7e

eskimor commented Feb 3, 2021

View reviewed changes

drahnr reviewed Feb 3, 2021

View reviewed changes

node/network/availability-distribution/src/lib.rs Outdated Show resolved Hide resolved

drahnr reviewed Feb 3, 2021

View reviewed changes

node/network/bridge/src/action.rs Show resolved Hide resolved

drahnr reviewed Feb 3, 2021

View reviewed changes

node/network/protocol/src/request_response.rs Outdated Show resolved Hide resolved

drahnr reviewed Feb 3, 2021

View reviewed changes

node/network/protocol/src/request_response.rs Show resolved Hide resolved

drahnr reviewed Feb 3, 2021

View reviewed changes

node/network/protocol/src/request_response.rs Outdated Show resolved Hide resolved

drahnr reviewed Feb 3, 2021

View reviewed changes

node/network/bridge/src/network.rs Outdated Show resolved Hide resolved

drahnr reviewed Feb 3, 2021

View reviewed changes

node/network/bridge/src/multiplexer.rs Outdated Show resolved Hide resolved

drahnr approved these changes Feb 3, 2021

View reviewed changes

eskimor and others added 4 commits February 3, 2021 16:06

Fix link

32cff58

Co-authored-by: Bernhard Schuster <bernhard@ahoi.io>

Use map_err.

9369937

Co-authored-by: Bernhard Schuster <bernhard@ahoi.io>

Improvements inspired by suggestions by @drahnr.

3877c07

- Channel size is now determined by function. - Explicitely scope NetworkService::start_request.

Merge branch 'rk-req-resp-2306' of github.com:paritytech/polkadot int…

f39e040

…o rk-req-resp-2306

ghost merged commit ecc3772 into master Feb 3, 2021

ghost deleted the rk-req-resp-2306 branch February 3, 2021 20:21

ordian mentioned this pull request Feb 4, 2021

End multiplexer stream once one of its inputs end. #2380

Merged

ordian added a commit that referenced this pull request Feb 4, 2021

merge master

96a3b03

* master: Diagnostics quality of life improvements (#2375) Generic request/response infrastructure for Polkadot (#2352) Fix bug in statement table (#2369)

eskimor mentioned this pull request Nov 11, 2022

Member request polkadot-fellows/seeding#57

Open

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generic request/response infrastructure for Polkadot #2352

Generic request/response infrastructure for Polkadot #2352

eskimor commented Jan 29, 2021 •

edited

Loading

ordian Feb 3, 2021

eskimor Feb 3, 2021

ordian Feb 4, 2021

eskimor Feb 4, 2021

ordian Feb 4, 2021

drahnr Feb 4, 2021 •

edited

Loading

eskimor Feb 4, 2021

ordian left a comment

eskimor Feb 3, 2021

drahnr Feb 3, 2021 •

edited

Loading

eskimor Feb 3, 2021

eskimor Feb 3, 2021

drahnr left a comment

eskimor commented Feb 3, 2021

ghost commented Feb 3, 2021

Generic request/response infrastructure for Polkadot #2352

Generic request/response infrastructure for Polkadot #2352

Conversation

eskimor commented Jan 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drahnr Feb 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ordian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drahnr Feb 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drahnr left a comment

Choose a reason for hiding this comment

eskimor commented Feb 3, 2021

ghost commented Feb 3, 2021

eskimor commented Jan 29, 2021 •

edited

Loading

drahnr Feb 4, 2021 •

edited

Loading

drahnr Feb 3, 2021 •

edited

Loading