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

Use patched ws-rs #583

Merged
merged 3 commits into from
Sep 18, 2020
Merged

Use patched ws-rs #583

merged 3 commits into from
Sep 18, 2020

Conversation

maciejhirsz
Copy link
Contributor

Revert to ws-rs once housleyjk/ws-rs#328 is merged and published.

dvdplm
dvdplm previously approved these changes Sep 17, 2020
ws/Cargo.toml Outdated
@@ -7,7 +7,7 @@ homepage = "https://github.com/paritytech/jsonrpc"
license = "MIT"
name = "jsonrpc-ws-server"
repository = "https://github.com/paritytech/jsonrpc"
version = "14.2.0"
version = "14.2.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Or should it be 14.3.0? cc @ordian

Copy link
Member

Choose a reason for hiding this comment

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

I think that's technically a breaking change (so should be 15.0.0) because of pub use parity_ws as ws; (it's also exposed in other public API): ws 0.9 and parity-ws 0.10 are two different (semver-incompatible) crates.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, seems the ws stuff is exposed for instance in middlewares, which for sure are used by openethereum (not necessarily substrate though).
Let's bump to 15.0.0 then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tomusdrw should I bump the version in the PR, and if so, should all crates be bumped to 15.0.0 or just the jsonrpc-ws-server?

Copy link
Contributor

Choose a reason for hiding this comment

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

should I bump the version in the PR, and if so, should all crates be bumped to 15.0.0 or just the jsonrpc-ws-server?

Yes and yes. We prefer to keep the major versions synchronized for simplicity even though there weren't any breaking changes.

Copy link
Member

Choose a reason for hiding this comment

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

would it make sense to create a v15.x branch for that instead of merging it to v14.x?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think not; I think the current v15 branch has unrelated and breaking changes. If this here becomes v15, then the current v15-branch will become v16 (correct me if I'm wrong @tomusdrw).

Copy link
Member

Choose a reason for hiding this comment

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

@dvdplm could you point me to the v15 branch? I don't see it listed here https://github.com/paritytech/jsonrpc/branches/all.
Or did you mean master? I think master can later become v16 if that's what you mean.

tomusdrw
tomusdrw previously approved these changes Sep 17, 2020
@@ -64,6 +64,8 @@ impl Server {
config.max_connections = max_connections;
// don't accept super large requests
config.max_fragment_size = max_payload_bytes;
config.max_in_buffer_capacity = max_payload_bytes;
Copy link
Contributor

Choose a reason for hiding this comment

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

Initially I thought that maybe it's too little, but the default 5MB for this should be more than enough for a bunch of requests.

@maciejhirsz
Copy link
Contributor Author

Bumped versions to 15.0.0 across the board, but merging this to the 14.x branch now doesn't make literal sense.

@maciejhirsz maciejhirsz changed the base branch from v14.x to v15.x September 18, 2020 18:48
@dvdplm dvdplm merged commit c97cce6 into v15.x Sep 18, 2020
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.

4 participants