Skip to content

Add hyper 0.12 based HTTP transport implementation #56

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

faern
Copy link
Member

@faern faern commented Feb 27, 2019

This is trying to move away from the now old tokio_core and hyper 0.11.

I'm planning on replacing jsonrpc_client_http completely with this new hyper 0.12 based transport. But as a transition while it's being developed I opted to keep both until we know the new one is usable. So the name newhttp is not something that I ever mean to publish or anything.

This implementation does not support custom headers. But for our use case we should not need that any more. We previously needed it for host headers when we connected directly to IPs. But with the new DNS cache solution we should be able to connect directly to our domains and no special hacks for SNI or host header insertion should be needed.

This transport is also a lot simpler. It does not contain any "client creator" code. And no helpers for creating hyper clients. The user just have to hand us any hyper::Client and we'll work with that directly.


This change is Reviewable

Copy link
Member Author

@faern faern left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 12 files reviewed, 1 unresolved discussion


newhttp/examples/simple.rs, line 14 at r1 (raw file):

    let jsonrpc_transport = jsonrpc_client_newhttp::HttpTransport::new(client, None);

    let uri = "https://api.mullvad.net/rpc/".parse::<Uri>().unwrap();

There is a bug somewhere, and I'm not able to track it down. If you change this URI into https://api.mullvad.net/rpc (remove the last /) then our server will return a 301 status code. This will result in this example binary to quit with:

[2019-02-27T13:41:05Z TRACE jsonrpc_client_newhttp] Sending request to https://api.mullvad.net/rpc
[2019-02-27T13:41:05Z DEBUG jsonrpc_client_newhttp] Got response with status 301 Moved Permanently
Error processing request: RPC Client already shut down

The entire transport is not supposed to exit just because it receives a 302 response. I have tried adding logging here and there in newhttp and I can't track down anything wrong. I suspect there might be something in core maybe?

Copy link
Contributor

@pinkisemils pinkisemils left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 12 files at r1.
Reviewable status: 6 of 12 files reviewed, 1 unresolved discussion

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