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

Switch from hyper to curl #344

Closed
brson opened this issue Apr 19, 2016 · 13 comments
Closed

Switch from hyper to curl #344

brson opened this issue Apr 19, 2016 · 13 comments

Comments

@brson
Copy link
Contributor

brson commented Apr 19, 2016

Hyper doesn't support HTTP proxies, while curl does.

@brson
Copy link
Contributor Author

brson commented Apr 19, 2016

Not sure if this should block an initial release, but it does seem important.

@mhristache
Copy link

A lot of companies are using proxies for internet access so I think this is very important.

@eminence
Copy link

the other option would be to teach hyper about proxies, but that seems like way more work than switching to curl

@mhristache
Copy link

FWIW, there is > 1 year old issue on hyper regarding proxy support in the client: hyperium/hyper#531

@xen0n
Copy link
Contributor

xen0n commented Apr 25, 2016

I'm definitely in favor of this because of the proxy support, but the curl crate declares an unstable API, also it didn't provide any notes on Windows support. However seeing the amount of work needed to make hyper support proxies it seems curl is the only way to go 😕

@peschkaj
Copy link
Contributor

Looks like curl does work on Windows, but maybe only with MSVC?
killercup/cargo-edit#55 (comment)

On Mon, Apr 25, 2016, 00:16 Wang Xuerui notifications@github.com wrote:

I'm definitely in favor of this because of the proxy support, but the curl
crate https://github.com/carllerche/curl-rust declares an unstable API,
also it didn't provide any notes on Windows support. However seeing the
amount of work needed to make hyper support proxies it seems curl is the
only way to go [image: 😕]


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#344 (comment)

Jeremiah Peschka

@brson
Copy link
Contributor Author

brson commented Apr 25, 2016

@peschkaj Cargo uses libcurl, so I'm confident we can make it work on all platforms that matter.

@seanmonstar
Copy link
Contributor

Proxy support is easy in the async branch, I can try to add basic support to hyper's sync branch to support this feature.

Basic functionality would be: when sending the request, if req.url.host() does not equal the Host header, then send the url in the absolute-uri format used for proxies. Support in the request builder could look like:

client.get("http://rustup.rs/path")
    .proxy(env::var("HTTP_PROXY").unwrap())
    .send()

Where HTTP_PROXY=http://our.corporate.proxy.tld

Would this work?

@seanmonstar
Copy link
Contributor

Or perhaps it makes more sense for the proxy to be on the Client...

let mut client = Client::new();
client.set_proxy(host, port);

@brson
Copy link
Contributor Author

brson commented Apr 25, 2016

@seanmonstar That works for me code-wise, though I don't know enough about the use cases to say it's sufficient. Thanks for jumping in.

@lilianmoraru
Copy link

@seanmonstar I think that's the simplest form.
For example, where I work, we use "WPAD", it's not set in "HTTP_PROXY".
Although, cargo doesn't work from the command line either because of WPAD...

@seanmonstar
Copy link
Contributor

seanmonstar commented Apr 25, 2016

@lilianmoraru it was just an example of rustup looking at an environment variable. Whichever way rustup wants to allow configuring a proxy is out of scope of hyper supporting a proxy.

This was referenced May 6, 2016
@brson
Copy link
Contributor Author

brson commented May 7, 2016

Now that TLS support is fixed and HTTP proxy support is on the way, I'm happy to stick with hyper. It'll be good to have such a prominent project out in production using it.

@brson brson closed this as completed May 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants