Allow swapping out native-tls for rustls

[aidanhs]

As far as I can see, there are three possible approaches to allow swapping out the ssl implementation:

1. Specify features in Cargo.toml that allow consuming users to swap out tls implementations. Advantage: swapping is as simple as default-features = false, features = ["rustls"] in Cargo.toml. Disadvantage: requires every possible desired alternative ssl implementation to be a feature (and multiplies features/parameters by N^2 for the other hyper::Client things users might want to tweak before construction).
2. Make a Client::new_with_tlsclient function, which takes a S: SslClient. Advantage: no hardcoding, still pretty simple to create. Disadvantage: still has the potential of multiplying by N^2 when users want to tweak other things.
3. Give the user an option to pass in a hyper::Client directly. Advantage: most flexible option. Disadvantage: most complex for the user (though they're probably a power user anyway), permits the user to possibly break some assumptions of reqwest (e.g. there's a client pool).

I think my preferred option is 3, but I'm not clear what the implications are of a user passing in a client that doesn't use a pool. Hopefully it'll just be slower?

Anyway, because of the uncertainty I implemented option 1 just to show willing :)

[seanmonstar]

I don't think option 1 can work, because of how cargo features are additive. They cannot be mutually exclusive. If one of your dependencies uses reqwest with the native-tls feature, and another uses it with the rustls feature, the crate can now longer compile, because the conditional compilation would mean the new_tlsclient would exist twice, and fail. Having different internal names wouldn't work either, and even if they could somehow, it'd be very ambiguous which was being used when a user called Client::new().

Option 3 is probably the one to use. I have some reservations, since that means the switch to hyper 0.11 will be even more breaking for power users, but I'm not sure a better way either...

[seanmonstar]

Why the open/close spam? XD

[aidanhs]

Sorry! I've pushed an updated PR, but travis had a spurious failure with linux and now seems to be taking forever to do OSX (https://travis-ci.org/seanmonstar/reqwest/pull_requests). I hoped that triggering via open/close would help, but it seems not.

I'll leave it overnight and check back tomorrow.

[aidanhs]

Oh wait what!? The status was showing as the build had taken 1hour and was still ongoing...but now it's done, apparently in 7min. I guess they had a backlog of notifications to get through. I'll be less impatient next time.

[seanmonstar]

Oh boy, flashbacks!

[aidanhs]

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.

[seanmonstar]

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...

[aidanhs]

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.

[seanmonstar]

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?

[aidanhs]

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?

[seanmonstar]

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.

[aidanhs]

I'm going to close this for now. I've worked around my issue with replace for now and I don't think the long-term solution is to require every single crate upstream from hyper to expose the gory internals of https choice - I've got some thoughts on a route forward but it's a somewhat more invasive ecosystem change.

Anyway, nobody's really delighted with this PR so let's see if a better route is possible outside of reqwest. I may be back if it falls through and let me know if anyone else is interested in reaching into the guts of hyper for configuration (as this PR is probably the most configurable way to do that) and I'll reopen.