Implement `ping-pong` for WebSocket clients #772

lexnv · 2022-05-17T16:00:43Z

This PR exposes the ping-pong interface of the WebSocket clients.

A new ping_interval configuration is exposed for async-clients.
The ping interval represents the amount of time that should be elapsed between
an activity event (frontend event, backend event / pong frame received)
and submitting a ping frame to the RPC server.

From the RPC: https://www.rfc-editor.org/rfc/rfc6455#section-5.5.2

Upon receipt of a Ping frame, an endpoint MUST send a Pong frame in
response, unless it already received a Close frame. It SHOULD
respond with Pong frame as soon as is practical.

It is possible that the Pong reply is not submitted in the expected time
(ie if the server received a state_getKeys request that is taking minutes to finish).
Therefore the ping_interval is set to 5 minutes to capture those cases.

Closes #738.

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

niklasad1 · 2022-05-17T16:42:28Z

hehe, the idea behind the issue was to implement server-side pings based on what Jaco told us about JS-side of things here

however, I think this is useful for the client as well.

niklasad1

Looks good overall 👍

It would be better to make the extra ping/pongs optional and disable it by default.
I think there a might be a use case where one wants avoid this extra overhead to send these extra messages (not exactly sure but currently it's not possible to turn it off except a very long ping interval)

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

client/transport/src/ws/mod.rs

core/src/client/async_client/mod.rs

client/transport/src/ws/mod.rs

core/src/client/mod.rs

client/transport/src/ws/mod.rs

core/src/client/async_client/mod.rs

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

core/src/client/async_client/mod.rs

TarikGul · 2022-05-27T01:41:13Z

core/src/client/async_client/mod.rs

+) -> Result<(), Error> {
+
+	// Handle raw messages of form `ReceivedMessage::Bytes` (Vec<u8>) or ReceivedMessage::Data` (String).
+	async fn handle_recv_message<S: TransportSenderT>(


So I am under the impression the reason this is nested is this fn is only used locally within the scope of handle_backend_messages? I quite like it actually as it distinquishes the difference between handling frontend messages and backend message. 👍 Nice cleanup from it earlier being inside of the fn background_task. It's way easier to read/digest.

TarikGul · 2022-05-27T01:51:23Z

core/src/client/async_client/mod.rs

+
+		Some(FrontToBack::Batch(batch)) => {
+			tracing::trace!("[backend]: client prepares to send batch request: {:?}", batch.raw);
+			// NOTE(niklasad1): annoying allocation.


@niklasad1 DQ: Just wondering, I've seen this before and have wanted to ask, what specifically about the clone allocation is annoying here?

we can remove that note but it's just the those are batch ids are already owned in this scope and so should be possible to borrow that in theory but we use the HashSet entry which takes ownership of that + RequestManager` doesn't allow borrows for simplicity.

core/src/client/async_client/mod.rs

TarikGul

👍 Great Job!

core/src/client/async_client/mod.rs

niklasad1

blocking this because of the trace logging thingy

core/Cargo.toml

Co-authored-by: Tarik Gul <47201679+TarikGul@users.noreply.github.com>

Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>

jsdw · 2022-05-27T10:03:16Z

client/transport/src/ws/mod.rs

@@ -195,6 +197,20 @@ impl TransportSenderT for Sender {
 		Ok(())
 	}

+	/// Sends out a ping request. Returns a `Future` that finishes when the request has been
+	/// successfully sent.


Since we've been talking about cancel safety a bit, and not really relaetd to this PR, but do you reckon it is worth adding an issue to add docs to functions to note when they aren't cancel safe @niklasad1?

(or maybe almost no methods are cancel safe, and we state that up front and then note which ones are, perhaps)

sounds like a good idea.

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

core/src/client/async_client/mod.rs

niklasad1

nice work, forgot about this one

lexnv added 6 commits May 17, 2022 16:49

ws: Implement ping for TransportSenderT trait

1162d3e

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

ws/client: Receive pong frames

2a76701

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

core/client: Use select! macro for the background task

80af45a

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

client: Propagate ping interval to background task

63ba7e7

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

async_client: Submit ping requests

0d3534c

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

async_client: Handle pong replies

23ad9b9

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

lexnv requested a review from a team as a code owner May 17, 2022 16:00

niklasad1 reviewed May 18, 2022

View reviewed changes

lexnv added 5 commits May 19, 2022 18:53

client: Handle frontend messages to dedicated fn

48b5db9

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

client: Handle backend messages in dedicated fn

c86b08e

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

client: Add terminated fuse for opt-out pings

ae5bf75

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

Set opt-out behavior for client pings

0ee78c5

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

Merge remote-tracking branch 'origin/master' into 738_ping_pong

8e82ad1

niklasad1 reviewed May 20, 2022

View reviewed changes

client/transport/src/ws/mod.rs Outdated Show resolved Hide resolved

niklasad1 reviewed May 20, 2022

View reviewed changes

core/src/client/async_client/mod.rs Show resolved Hide resolved

niklasad1 reviewed May 20, 2022

View reviewed changes

core/src/client/async_client/mod.rs Outdated Show resolved Hide resolved

niklasad1 reviewed May 20, 2022

View reviewed changes

core/src/client/async_client/mod.rs Outdated Show resolved Hide resolved

niklasad1 reviewed May 20, 2022

View reviewed changes

client/transport/src/ws/mod.rs Outdated Show resolved Hide resolved

niklasad1 reviewed May 20, 2022

View reviewed changes

core/src/client/mod.rs Outdated Show resolved Hide resolved

niklasad1 reviewed May 20, 2022

View reviewed changes

client/transport/src/ws/mod.rs Outdated Show resolved Hide resolved

niklasad1 reviewed May 20, 2022

View reviewed changes

client/transport/src/ws/mod.rs Outdated Show resolved Hide resolved

niklasad1 reviewed May 20, 2022

View reviewed changes

core/src/client/async_client/mod.rs Outdated Show resolved Hide resolved

lexnv added 7 commits May 20, 2022 12:25

client: Move imports

e7c6edb

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

client: Handle handle_frontend_messages errors

9c5f235

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

client: Add custom error related to byteslice conversions

8e3ff40

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

client: Modify send_ping to send empty slices

7d39b6f

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

Fix cargo hack check and use select_biased

64a7b99

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

Handle sending pings with lowest priority

1559554

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

core: Add proper number of params to background_task

5ba4d0e

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

TarikGul reviewed May 27, 2022

View reviewed changes

core/src/client/async_client/mod.rs Outdated Show resolved Hide resolved

TarikGul reviewed May 27, 2022

View reviewed changes

core/src/client/async_client/mod.rs Outdated Show resolved Hide resolved

TarikGul reviewed May 27, 2022

View reviewed changes

core/src/client/async_client/mod.rs Outdated Show resolved Hide resolved

TarikGul reviewed May 27, 2022

View reviewed changes

core/src/client/async_client/mod.rs Outdated Show resolved Hide resolved

TarikGul approved these changes May 27, 2022

View reviewed changes

niklasad1 self-requested a review May 27, 2022 08:00

niklasad1 reviewed May 27, 2022

View reviewed changes

core/src/client/async_client/mod.rs Outdated Show resolved Hide resolved

niklasad1 requested changes May 27, 2022

View reviewed changes

niklasad1 reviewed May 27, 2022

View reviewed changes

core/Cargo.toml Outdated Show resolved Hide resolved

niklasad1 reviewed May 27, 2022

View reviewed changes

core/Cargo.toml Outdated Show resolved Hide resolved

lexnv and others added 7 commits May 27, 2022 12:34

Update core/src/client/async_client/mod.rs

2d6b04c

Co-authored-by: Tarik Gul <47201679+TarikGul@users.noreply.github.com>

Update core/src/client/async_client/mod.rs

24b5d67

Co-authored-by: Tarik Gul <47201679+TarikGul@users.noreply.github.com>

Update core/src/client/async_client/mod.rs

839ec65

Co-authored-by: Tarik Gul <47201679+TarikGul@users.noreply.github.com>

Update core/src/client/async_client/mod.rs

8a1462a

Co-authored-by: Tarik Gul <47201679+TarikGul@users.noreply.github.com>

Update core/src/client/async_client/mod.rs

ba14af9

Co-authored-by: Tarik Gul <47201679+TarikGul@users.noreply.github.com>

Update core/Cargo.toml

b79c64f

Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>

Update core/Cargo.toml

e615af5

Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>

jsdw reviewed May 27, 2022

View reviewed changes

lexnv added 2 commits May 27, 2022 13:03

logs: Keep debug log for submitting Ping frames

baab48f

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

Print debug logs when receiving Pong frames

1738722

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

jsdw approved these changes May 27, 2022

View reviewed changes

lexnv requested a review from niklasad1 May 27, 2022 10:29

niklasad1 reviewed Jun 1, 2022

View reviewed changes

core/src/client/async_client/mod.rs Outdated Show resolved Hide resolved

niklasad1 approved these changes Jun 1, 2022

View reviewed changes

Update core/src/client/async_client/mod.rs

c661065

lexnv merged commit a0813cb into master Jun 1, 2022

lexnv deleted the 738_ping_pong branch June 1, 2022 13:52

lexnv mentioned this pull request Jun 1, 2022

[ws client] Disconnect client when Pong frames are missing #787

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `ping-pong` for WebSocket clients #772

Implement `ping-pong` for WebSocket clients #772

lexnv commented May 17, 2022

niklasad1 commented May 17, 2022

niklasad1 left a comment •

edited

Loading

TarikGul May 27, 2022

TarikGul May 27, 2022

niklasad1 May 27, 2022

TarikGul left a comment

niklasad1 left a comment

jsdw May 27, 2022 •

edited

Loading

niklasad1 Jun 1, 2022

niklasad1 left a comment

Implement ping-pong for WebSocket clients #772

Implement ping-pong for WebSocket clients #772

Conversation

lexnv commented May 17, 2022

niklasad1 commented May 17, 2022

niklasad1 left a comment • edited Loading

Choose a reason for hiding this comment

TarikGul May 27, 2022

Choose a reason for hiding this comment

TarikGul May 27, 2022

Choose a reason for hiding this comment

niklasad1 May 27, 2022

Choose a reason for hiding this comment

TarikGul left a comment

Choose a reason for hiding this comment

niklasad1 left a comment

Choose a reason for hiding this comment

jsdw May 27, 2022 • edited Loading

Choose a reason for hiding this comment

niklasad1 Jun 1, 2022

Choose a reason for hiding this comment

niklasad1 left a comment

Choose a reason for hiding this comment

Implement `ping-pong` for WebSocket clients #772

Implement `ping-pong` for WebSocket clients #772

niklasad1 left a comment •

edited

Loading

jsdw May 27, 2022 •

edited

Loading