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

Redo http transport #497

Merged
merged 2 commits into from
Jun 8, 2021
Merged

Redo http transport #497

merged 2 commits into from
Jun 8, 2021

Conversation

e00E
Copy link
Contributor

@e00E e00E commented Jun 4, 2021

The existing implementation has accumulated a lot of crust from initial
async support and use of hyper. Now that is using reqwest we can do many
things simpler.

Removed things that were needed for hyper but are handled already by
reqwest:

  • setting the proxy
  • setting the auth header
  • setting the content type header
  • setting content length header

Other changes:

  • Move from struct with Future impl to BoxFuture. We were already using
    BoxFuture internally anyawy.
  • Log request and response as debug strings. This makes sure no control
    characters like newlines end up in the output.
  • Use from_utf8_lossy for the response log so that we can still
    partially log non utf8 responses.
  • Fix not handling batch responses according to the jsonrpc
    specification which does not guarantee the ordering of the list.
  • Add more context to error logs.

Copy link
Owner

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

Nice!

src/transports/http.rs Show resolved Hide resolved
src/transports/http.rs Show resolved Hide resolved
src/transports/http.rs Show resolved Hide resolved
Copy link
Owner

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

Looks good, thanks a lot! The WASM build requires a tiny fix.

The existing implementation has accumulated a lot of crust from initial
async support and use of hyper. Now that is using reqwest we can do many
things simpler.

Removed things that were needed for hyper but are handled already by
reqwest:
- setting the proxy
- setting the auth header
- setting the content type header
- setting content length header

Other changes:
- Move from struct with Future impl to BoxFuture. We were already using
BoxFuture internally anyawy.
- Log request and response as debug strings. This makes sure no control
characters like newlines end up in the output.
- Use from_utf8_lossy for the response log so that we can still
partially log non utf8 responses.
- Fix not handling batch responses according to the jsonrpc
specification which does not guarantee the ordering of the list.
- Add more context to error logs.
- Allow giving a custom client to the transport. This is useful for
example to set a timeout.
@e00E
Copy link
Contributor Author

e00E commented Jun 7, 2021

Looks like you can't set a user agent on wasm, which makes sense as the browser probably uses its own. Let's see if this fixes the build.

Copy link
Owner

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

Awesome, one last bit is WASM-build warning, and we're good to go :)

src/transports/http.rs Outdated Show resolved Hide resolved
src/transports/http.rs Outdated Show resolved Hide resolved
Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com>
@tomusdrw tomusdrw merged commit 22ac718 into tomusdrw:master Jun 8, 2021
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.

2 participants