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.

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

ws server: respect max limit for received messages #537

Merged
merged 21 commits into from
Nov 9, 2021

Conversation

niklasad1
Copy link
Member

@niklasad1 niklasad1 commented Oct 20, 2021

This PR changes such that we use the max limit for the received messages in soketto (WS) to deny too large message and not allocate a huge buffer that we did before.

ws-server/src/server.rs Outdated Show resolved Hide resolved
ws-server/src/server.rs Outdated Show resolved Hide resolved
ws-server/src/server.rs Outdated Show resolved Hide resolved
ws-server/src/server.rs Outdated Show resolved Hide resolved
ws-server/src/server.rs Show resolved Hide resolved
@niklasad1
Copy link
Member Author

I think we should continue logging this, but perhaps more granularly?

Ok, I'm convinced to keep the logs as errors but the logs are already sufficiently granular..

For example:

ERROR jsonrpsee_ws_server::server: WS recv error: Io(Os { code: 107, kind: NotConnected, message: "Transport endpoint is not connected" }) => terminate connection 0

@dvdplm
Copy link
Contributor

dvdplm commented Oct 21, 2021

but the logs are already sufficiently granular..

Fair. By matching more granularly we might be able to remove some of them that aren't relevant errors to the user, but I don't think there are any really?

@niklasad1
Copy link
Member Author

Fair. By matching more granularly we might be able to remove some of them that aren't relevant errors to the user, but I don't think there are any really?

I agree, there are none except maybe some I/O error's because the connection was already closed by the peer.

@niklasad1
Copy link
Member Author

Blocked on paritytech/soketto#53

@niklasad1 niklasad1 changed the title ws server: don't kill connection max limit exceeds ws server: respect max limit for received messages Nov 8, 2021
@niklasad1 niklasad1 requested review from maciejhirsz and jsdw November 8, 2021 14:22
let subscriber = tracing_subscriber::FmtSubscriber::builder().finish();
tracing::subscriber::set_global_default(subscriber).expect("setting default subscriber failed");
tracing_subscriber::FmtSubscriber::builder()
.with_env_filter(tracing_subscriber::EnvFilter::from_default_env())
Copy link
Member Author

Choose a reason for hiding this comment

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

use RUST_LOG env variable

Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

A few nits. Still LGTM.

ws-server/src/server.rs Outdated Show resolved Hide resolved
ws-server/src/server.rs Outdated Show resolved Hide resolved
ws-server/src/server.rs Outdated Show resolved Hide resolved
ws-server/src/server.rs Outdated Show resolved Hide resolved
ws-server/src/server.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@maciejhirsz maciejhirsz left a comment

Choose a reason for hiding this comment

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

Nice!

// TODO: check length of response https://github.com/paritytech/jsonrpsee/issues/536
tracing::debug!("send {} bytes", response.len());
tracing::trace!("send: {}", response);
let _ = sender.send_text_owned(response).await;
Copy link
Contributor

@maciejhirsz maciejhirsz Nov 9, 2021

Choose a reason for hiding this comment

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

👍 for skipping a clone :)

@dvdplm dvdplm merged commit 6dac20d into master Nov 9, 2021
@dvdplm dvdplm deleted the na-ws-dont-terminate-conn-exceed-max-limit branch November 9, 2021 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants