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

Uniform API for custom headers between clients #814

Merged
merged 13 commits into from
Jul 13, 2022

Conversation

lexnv
Copy link
Contributor

@lexnv lexnv commented Jul 5, 2022

Jsonrpsee allows users to set custom headers during the WS handshake (for the ws-clients).

This PR extends the ability to add custom headers for the HTTP clients.

The PR introduces a uniform API for the client builders, relying on http::HeaderMap

pub fn set_headers(mut self, headers: http::HeaderMap) -> Self

that behaves as follows:

  • ws-client: Custom headers are used only during the handshake
  • http-client: Custom headers are submitted with every request

One advantage is that the ws-client and http-client have a uniform API, used for caching or any custom user operations, extending jsonrpsee beyond the realm of the Substrate.
One disadvantage is that ws-client uses soketto that internally requires an httparse::Header object, which leads to conversion from http::HeaderMap only if present.

Next Steps

  • Write documentation to illustrate differences between ws/http clients

Closes #809.

lexnv added 4 commits July 4, 2022 17:35
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv requested a review from a team as a code owner July 5, 2022 14:41
@lexnv lexnv self-assigned this Jul 5, 2022
lexnv added 3 commits July 5, 2022 20:20
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

LGTM,

I like this and I have created this PR https://github.com/paritytech/soketto/pull/60/files which changes soketto to use HeaderMap but I need to benchmark it.

Especially for the client is makes sense as to have this verification as invalid HTTP headers can't be created.

httparse is really intended for parsing HTTP headers not create them :P

lexnv added 2 commits July 6, 2022 13:54
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Copy link
Collaborator

@jsdw jsdw left a comment

Choose a reason for hiding this comment

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

Liking the standardisation! Any idea what the performance impact of this change is?

@niklasad1
Copy link
Member

For the websocket client is should "only" impact the connection speed and we have no benchmarks for that....

On the other hand for the HTTP client the headers are "cloned" for each call which should visible on the benchmarks if we enabled "some custom headers".

@lexnv
Copy link
Contributor Author

lexnv commented Jul 7, 2022

The results of running the benchmarks surprisingly improved performance for some scenarios.
However, there are some cases where performance was below the accepted threshold of the criterion.

Scenario Run 1 Run 2
async/http_batch_requests/memory_intense/50 -6.5777% +54.263%
async/ws_batch_requests/memory_intense/100 +5.9838% +66.996%

The test runs were performed a few hours apart, without changing the commit head (this one).
The difference between the test runs suggests that my machine is not quite reliable. Went from a negative difference (improvement) to positive (regression) with more than 55% between runs.

A few other outliers

# Regressions
async/http_batch_requests/fast_call/5      +7.2570%
async/http_batch_requests/fast_call/100  +16.523%
sync/ws_concurrent_conn_calls/4             +6.3084%

# Improvements
async/ws_concurrent_conn_calls/8                     -4.6004%
async/ws_concurrent_conn_calls/2                     -4.7548%
async/http_concurrent_conn_calls/fast_call/2     -15.231%

That being said, I think we'll get a more reliable read of the benchmarks running the nightly CI.

@niklasad1
Copy link
Member

niklasad1 commented Jul 7, 2022

The batch benches are really "random", so nothing to with your PR but probably worth to investigate why that is.

The HTTP requests could be faster as the actual request headers are "cached" and cloned on each request which wasn't the case before.

However, it would be good to add two additional benches:

  1. WS connection time with different size of headers
  2. HTTP calls with different size of headers

Feel free to merge this one but I don't think the current benches really matter that much for this PR.

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv merged commit 0ccfbd7 into master Jul 13, 2022
@lexnv lexnv deleted the 809_uniform_custom_headers branch July 13, 2022 12:02
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.

[clients]: unify APIs to configure custom headers
3 participants