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.

Sign up for GitHub

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

Jump to bottom

client: Implement notify_on_disconnect #837

Merged
merged 16 commits into from
Aug 16, 2022
Merged

client: Implement notify_on_disconnect #837

merged 16 commits into from
Aug 16, 2022

Conversation

lexnv
Copy link
Contributor

@lexnv lexnv commented Aug 2, 2022 •
edited
Loading

This PR introduces the client's notify_on_disconnect functionality.

When called, the method completes when the client is disconnected
or the client's background task encountered an error.
The method is cancel-safe, as the client disconnects or background task
errors are unrecoverable.

Testing Done

  • Server: Started a process with WS server that terminates in 60 seconds
  • Client: Submitted a request (to verify connectivity) and called client.notify_on_disconnect().await;

Logs

Server

2022-08-02T10:00:37.479952Z DEBUG jsonrpsee_ws_server::server: recv pong
2022-08-02T10:00:37.601751Z  INFO ws_server: Server Shutdown

Client

2022-08-02T10:00:37.604050Z ERROR jsonrpsee_core::client::async_client: Error: Connection(Closed) terminating client
2022-08-02T10:00:37.604176Z  WARN jsonrpsee_core::client::async_client: Transport(WebSocket connection error: connection closed

Caused by:
    connection closed)
2022-08-02T10:00:37.604617Z TRACE soketto::connection: ebce45eb: closing connection
2022-08-02T10:00:37.604655Z TRACE soketto::connection: ebce45eb: send: (Close (fin 1) (rsv 000) (mask (1 33031046)) (len 2))
2022-08-02T10:00:37.605058Z TRACE soketto::connection: ebce45eb: Sender flushing connection
2022-08-02T10:00:37.605205Z TRACE mio::poll: deregistering event source from poller
2022-08-02T10:00:37.605375Z  INFO ws_client: Client stop...

Next

  • Handle tokio::SendError and futures_utils::SendError conversion
  • Handle WS part

Closes #770.

@lexnv
client: Implement notify_on_disconnect
c8ffaa4 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv requested a review from a team as a code owner August 2, 2022 10:02
lexnv added 3 commits August 2, 2022 14:51
@lexnv
Revert "client: Implement notify_on_disconnect"
b21686b 
This reverts commit c8ffaa4.
@lexnv
client: Rely on tokio::sync::mpsc::Sender to notify on disconnect
5353b7d 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
Implement InternalError for tokio and futures_channel's SendError
17415ec 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv self-assigned this Aug 3, 2022
lexnv added 2 commits August 3, 2022 15:49
@lexnv
client: Remove tokio-stream dependency
7695c57 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
client: Rename notify_on_disconnect to on_disconnect
0850927 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
niklasad1
niklasad1 reviewed Aug 3, 2022
View reviewed changes
core/src/client/async_client/mod.rs
/// Completes when the client is disconnected or the client's background task encountered an error.
/// If the client is already disconnected, the future produced by this method will complete immediately.
///
/// # Cancel safety
Copy link
Member

Choose a reason for hiding this comment

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

this is really great, we should adopt this idiom IMO to document cancel safety.

most things here are cancel-safe but really great to be explicit.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed! It's so useful to know :)

lexnv added 3 commits August 3, 2022 17:16
@lexnv
Add tokio/sync as dependency of client feature
010f486 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
wasm-client: Use tokio::sync::mpsc::channel for notify on disconnect
b0886e5 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
Add tokio/sync dependency to satisfy --no-default-features check
c442bd4 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
jsdw
jsdw reviewed Aug 3, 2022
View reviewed changes
core/Cargo.toml Outdated Show resolved Hide resolved
jsdw
jsdw reviewed Aug 3, 2022
View reviewed changes
core/src/error.rs Outdated Show resolved Hide resolved
jsdw
jsdw reviewed Aug 3, 2022
View reviewed changes
core/src/error.rs Outdated Show resolved Hide resolved
@lexnv
Best effort to close channel on client drops
0975438 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@jsdw
Copy link
Collaborator

jsdw commented Aug 3, 2022

One other thing; can we add a couple of tests for this new function?

lexnv added 6 commits August 4, 2022 17:33
@lexnv
revert to: client: Implement notify_on_disconnect
5f6ae03 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
Replace tokio/sync with oneshot channel
2e29a9a 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
tests: Check client.on_disconnect() returns when server shuts down
746fc2a 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
tests: Fix comment typo
c47b5cd 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
tests: Call on_disconnect() multiple times
ba76226 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
tests: Call on_disconnect with closed server
5dbfa50 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
niklasad1
niklasad1 reviewed Aug 8, 2022
View reviewed changes
core/src/client/async_client/mod.rs
});
Client {
to_back,
request_timeout: self.request_timeout,
error: Mutex::new(ErrorFromBack::Unread(err_rx)),
id_manager: RequestIdManager::new(self.max_concurrent_requests, self.id_kind),
max_log_length: self.max_log_length,
notify: Mutex::new(Some(on_close_rx)),
Copy link
Member

Choose a reason for hiding this comment

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

no more tokio::sync::Notify?
doesn't it compile for WASM or what's the reason?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This perhaps?: #837 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, that's the explanation, mainly to reduce dependencies on tokio 🙏

@lexnv lexnv merged commit 6fdb67f into master Aug 16, 2022
@lexnv lexnv deleted the 770_notify_on_disconnect branch August 16, 2022 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@niklasad1 niklasad1 niklasad1 approved these changes

@jsdw jsdw jsdw approved these changes

Assignees

@lexnv lexnv

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[async client]: add notify_on_disconnect hook when the connection is closed
3 participants
@lexnv @jsdw @niklasad1