refactor: replace soketto with yawc for per-message deflate support - #1627
refactor: replace soketto with yawc for per-message deflate support#1627ifdario wants to merge 1 commit into
Conversation
Migrate from soketto to yawc across the entire codebase (server, client transport, ws-client, test-utils). yawc provides RFC 7692 permessage-deflate compression with balanced compression enabled by default, near-zero-copy frame processing, and SIMD-optimized masking. Key changes: - Replace soketto handshake/connection with yawc WebSocket::upgrade (server) and WebSocket::handshake_with_request (client) - Remove tokio-util compat layer (yawc uses tokio I/O natively) - Add configurable WsOptions (compression level, payload limits) to ServerConfigBuilder, WsTransportClientBuilder, and WsClientBuilder - Re-export DeflateOptions and WsOptions from server and client crates - Update test mocks to use yawc with safe Arc<Mutex<Option>> pattern - Adapt max request body size test for yawc's connection-terminating behavior on oversized payloads
|
Thankyou for your PR. For full context, I can see that That said, while I'm curious to see how far this PR would get on our CI run, we don't have much capacity for reviewing such a change (especially as we don't require it for the current jsonrpsee use cases), and we also own |
|
Hello @jsdw I have been planning this PRs for quite some time, waiting for yawc to be stable enough. I consider that yawc has reached this point. Feel free to leave the PR open if you see no use case or you would like to leave it for the future. |
|
Would be cool if you could run the benchmarks on this branch and compare to master, it seems that you don't have any benches against soketto in yawc. Nice that all tests passes but it's up to @jsdw to decide as the solo maintainer of jsonrpsee these days 😀 |
|
I've tried this yawc-based implementation (without performance comparison yet); it works pretty stably, including compression. Soketto looks abandoned: the last changes were made 2 years ago, and critical compression issues have persisted there for years (paritytech/soketto#49). However, I don't think maintainers will replace the implementation, since Soketto is developed by the same org. So, this is a conflict of interest. |
| /// let ws_opts = WsOptions::default().with_high_compression(); | ||
| /// let builder = ServerConfigBuilder::default().set_ws_options(ws_opts); | ||
| /// ``` | ||
| pub fn set_ws_options(mut self, options: yawc::Options) -> Self { |
There was a problem hiding this comment.
It's a leaky abstraction:
- If
yawcis updated (it's stillv0.x), it will affect the version of jsonrpsee. For instance, thisyawc::Optionsis not evennon_exhaustive, so even adding a new option is a breaking change (raising questions about the current state ofyawcAPI). - It makes it more complicated to switch backend or support a new one.
I can't say that jsonrpsee is a well-designed API, quite the opposite. But it would be great to avoid unfounded breaking changes in the future
| let response = client.send_request_text(req).await.unwrap(); | ||
| let response = client2.send_request_text(req).await.unwrap(); | ||
| assert_eq!(response, ok_response(JsonValue::String("a".repeat(100)), Id::Num(1))); | ||
|
|
There was a problem hiding this comment.
I think the integration test (like https://github.com/paritytech/jsonrpsee/pull/1632/changes#diff-8f746fec0e7d22931bdfb823ebec4c4079430e50bce5b4a795f9972207d239cdR1609) should be added to check compression
|
|
||
| /// Set custom WebSocket options such as compression settings. | ||
| /// | ||
| /// By default, balanced compression is enabled. The `max_payload_read` setting |
There was a problem hiding this comment.
By default, balanced compression is enabled
I think custom Options can have this in the Default instance instead of .unwrap_or() handling in the code.
|
|
||
| /// Set custom WebSocket options such as compression settings. | ||
| /// | ||
| /// By default, balanced compression is enabled. The `max_payload_read` setting |
There was a problem hiding this comment.
from these options will be overridden by [
ServerConfigBuilder::max_request_body_size].
... which is set by default. One more point that it shouldn't accept yawc::Options directly
|
Apologies; I am leaving Parity myself tomorrow and so will pass on mention of these PRs internally and hopefully somebody else can pick them up! |
4 participants
Migrate from soketto to yawc across the entire codebase (server, client transport, ws-client, test-utils). yawc provides RFC 7692 permessage-deflate compression with balanced compression enabled by default, near-zero-copy frame processing, and SIMD-optimized masking.
Key changes: