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

Comments

Projects
None yet
7 participants
@brson
Copy link
Contributor

brson commented Apr 19, 2016

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

@brson

This comment has been minimized.

Copy link
Contributor Author

brson commented Apr 19, 2016

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

@mhristache

This comment has been minimized.

Copy link

mhristache commented Apr 24, 2016

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

@eminence

This comment has been minimized.

Copy link

eminence commented Apr 24, 2016

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

@mhristache

This comment has been minimized.

Copy link

mhristache commented Apr 24, 2016

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

@xen0n

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

peschkaj commented Apr 25, 2016

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

seanmonstar commented Apr 25, 2016

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

This comment has been minimized.

Copy link
Contributor

seanmonstar commented Apr 25, 2016

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

This comment has been minimized.

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

This comment has been minimized.

Copy link

lilianmoraru commented Apr 25, 2016

@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

This comment has been minimized.

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

This comment has been minimized.

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 May 7, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.