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.

Sign up for GitHub

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

Allow swapping out native-tls for rustls #63

Closed
wants to merge 1 commit into from
Closed

Allow swapping out native-tls for rustls #63

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .appveyor.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,5 @@ install:
build: false
test_script:
- cargo build --verbose
- cargo build --verbose --no-default-features
- cargo test --verbose
1 change: 1 addition & 0 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ cache:

script:
- cargo build --verbose
- cargo build --verbose --no-default-features
- cargo test --verbose

notifications:
Expand Down
6 changes: 5 additions & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ categories = ["web-programming::http-client"]

[dependencies]
hyper = "0.10.2"
hyper-native-tls = "0.2"
hyper-native-tls = { version = "0.2", optional = true }
Copy link
Owner

Choose a reason for hiding this comment

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

Oh boy, flashbacks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if this is a hint that you're expecting this change to not be present!

I'll clarify - the main motivation for this PR for me is to remove the dependency on openssl, so above all else I need to make hyper-native-tls optional. Of course, this has knock-on effects - if I remove hyper-native-tls, I need another way to support tls (i.e. hyper-rustls).

Fortunately, making hyper-native-tls optional works fine under the additive model (which I didn't really know about, so thanks for mentioning it) - if one consumer doesn't require hyper-native-tls, the way it constructs a reqwest client is totally independent of whether hyper-native-tls is actually present or not, since it uses a completely different function.

Copy link
Owner

Choose a reason for hiding this comment

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

No, not objecting really. Just remembering the joys of when TLS was optional in hyper...

I do kind of wish this was pushed for instead in the native-tls repo, so that native-tls resorts to rustls on Linux instead of OpenSSL...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me think about what it would take to get this into native-tls instead (perhaps as a feature, rather than by default). It would potentially push the problem down the road (e.g. complex proxy configurations), but those might be better handled as first-class citizens in reqwest anyway, and rustls would be useful to have in native-tls anyway.

log = "0.3"
serde = "0.9"
serde_json = "0.9"
Expand All @@ -21,3 +21,7 @@ libflate = "0.1.3"

[dev-dependencies]
env_logger = "0.3"

[features]
default = ["default-tls"]
default-tls = ["hyper-native-tls"]
12 changes: 10 additions & 2 deletions src/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,16 +32,23 @@ pub struct Client {

impl Client {
/// Constructs a new `Client`.
#[cfg(feature = "default-tls")]
pub fn new() -> ::Result<Client> {
let mut client = try!(new_hyper_client());
client.set_redirect_policy(::hyper::client::RedirectPolicy::FollowNone);
Ok(Client {
Ok(Client::with_hyper_client(client))
}

/// Constructs a new `Client` with the provided `::hyper::Client` without
/// altering any settings.
Copy link
Owner

Choose a reason for hiding this comment

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

Should this be promised? Specifically I'm thinking about the RedirectPolicy. hyper's Client has a default policy that is, well, less correct. Would it be terrible if we unconditionally disabled the redirect policy on any hyper Client received?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know to be honest. I was considering a note like "if you use this method, you should look at the source of reqwest to create a client that's configured the same way as reqwest is expecting".

My main question is whether someone will someday want to use the hyper redirect policy rather than the reqwest one - what happens then? Or some other method on hyper that reqwest wants to set a default for (e.g. reqwest wants to start configuring timeouts and wants to set a different default value to hyper)?

How about I that "this method will configure your hyper::Client to use the reqwest default settings", and then if someone doesn't want that then they can make a with_hyper_client_raw method or similar?

Copy link
Owner

Choose a reason for hiding this comment

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

I guess the provided client should be left alone, then. I wouldn't want an extra _raw method. Perhaps best to point out that reqwest won't adjsut the Client, even though it normally would alter the defaults in the new constructor.

pub fn with_hyper_client(client: ::hyper::Client) -> Client {
Client {
inner: Arc::new(ClientRef {
hyper: client,
redirect_policy: Mutex::new(RedirectPolicy::default()),
auto_ungzip: AtomicBool::new(true),
}),
})
}
}

/// Enable auto gzip decompression by checking the ContentEncoding response header.
Expand Down Expand Up @@ -112,6 +119,7 @@ struct ClientRef {
auto_ungzip: AtomicBool,
}

#[cfg(feature = "default-tls")]
fn new_hyper_client() -> ::Result<::hyper::Client> {
use hyper_native_tls::NativeTlsClient;
Ok(::hyper::Client::with_connector(
Expand Down
2 changes: 2 additions & 0 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ extern crate hyper;

#[macro_use] extern crate log;
extern crate libflate;
#[cfg(feature = "default-tls")]
extern crate hyper_native_tls;
extern crate serde;
extern crate serde_json;
Expand Down Expand Up @@ -143,6 +144,7 @@ mod redirect;
/// reqwest::get("https://www.rust-lang.org").unwrap()
/// .read_to_string(&mut result);
/// ```
#[cfg(feature = "default-tls")]
pub fn get<T: IntoUrl>(url: T) -> ::Result<Response> {
let client = try!(Client::new());
client.get(url).send()
Expand Down