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

compatibility with http crate #1313

Closed
srijs opened this issue Sep 9, 2017 · 12 comments
Closed

compatibility with http crate #1313

srijs opened this issue Sep 9, 2017 · 12 comments
Labels
C-feature Category: feature. This is adding a new feature. E-hard Effort: hard. Likely requires a deeper understanding of how hyper's internals work.

Comments

@srijs
Copy link
Contributor

srijs commented Sep 9, 2017

Now that the http crate is officially published, are there plans to provide a compatibility layer for hyper, or will this have to wait for hyper 0.12?

If you're open to the former, I have some glue code I'd be willing to contribute.

@jimmycuadra
Copy link

I was thinking about starting on this work too. My understanding was that the plan was to replace all the existing hyper types with ones from the http crate. Is there a more specific plan or is there any code underway already? Or can anyone from the community just pick this up?

@carllerche
Copy link

I believe that is the plan, but doing so would require a breaking change (thus a 0.x bump).

@srijs
Copy link
Contributor Author

srijs commented Sep 9, 2017

I think one question might be if there is a way to introduce parts of the http crate w/o breaking the API just yet? Two possible approaches I could think of:

  • Introduce conversion/compatibility methods, e.g. to be able to pass a Service using http types to Http::bind (or an equivalent compat. method).

  • Replace some of the internal logic with pieces from http (url parsing comes to mind as an example), making existing hyper types a lightweight facade on top of http types so that the next step of replacing them for the 0.12 release would be easier (all the while reaping the benefits of less code in hyper, and getting validation on the http logic).

@jimmycuadra
Copy link

Personally, I'd much rather see hyper's (now redundant) types disappear completely in 0.12. My impression is that people using Rust and hyper at this point understand that the HTTP stack is not stable yet, and that making breaking changes where it's clearly needed is expected in order to push us towards 1.0.

@srijs
Copy link
Contributor Author

srijs commented Sep 10, 2017

I think we both are in agreement that that's what should happen for 0.12. (Assuming @seanmonstar making that call of course).

Depending on what the time table around this is though, I think it might be beneficial to offer some level of compatibility before introducing 0.12.

@seanmonstar seanmonstar modified the milestone: 0.12 Sep 11, 2017
@seanmonstar
Copy link
Member

We do expect 0.12 to remove hyper's related types, and use http instead. As noted, that's a breaking change, so we'd like to group that with other breaking changes that will likely need to happen, like changes to allow h2 to work.

The idea mentioned here is neat, though: providing compatibility with http in hyper 0.11. That could perhaps allow downstream users and frameworks to start adopting http sooner, and then an when hyper 0.12 releases, that part is an easier change. I like it!

@seanmonstar
Copy link
Member

See #1317 for the breaking change issue, but we can keep discussion in here for how to provide compatibility in 0.11!

@srijs
Copy link
Contributor Author

srijs commented Sep 12, 2017

@seanmonstar Thanks for your input! If you don't mind I'd like your feedback on the following:

So far I can see two different approaches, each with their pros and cons on how we can achieve this. It might also make sense to mix the two approaches, depending on how well they work for the different types.

EDIT: Although I initially favoured Option 2, after playing around with the code base it seems to be a lot harder to implement than I originally thought due to various reasons (the biggest one being a feature mismatch between the types in hyper and http). So my suggestion would be to go with Option 1 in the beginning, and then optionally work on porting the missing features over to http.

Option 1: Conversion functions

Where it makes sense, provide from and into_* methods on hyper types (such as Url, Headers, Request, etc. These methods convert between hyper and http types.

Pros

  • Least intrusive set of changes
  • Can be hidden behind feature flag if out-of-the-box dependency on http is not desired

Cons

  • Performance overhead when converting, which might be a deterrent for downstream users to adopt the new feature

Option 2: http types at the center, hyper types as the facade

Instead of providing conversion functions, change the hyper types to wrap the http version of the type, and change the method implementations to proxy through and apply the necessary changes for backwards compatibility.

Provide wrap/unwrap methods to change between the types.

Pros

  • Virtually no performance overhead
  • Start using the implementations from http (parsers, etc), so they can be hardened before hyper 0.12.
  • Possibly a reduction in code size for hyper (since parsing/validation logic etc can be removed in quite a few places)

Cons

@seanmonstar
Copy link
Member

Where it makes sense, provide from and into_* methods on hyper types (such as Url, Headers, Request, etc. These methods convert between hyper and http types.

This seems like a good idea regardless, since none of the types can be replaced completely.

Possibly difficult to establish backwards compatibility in some cases

This would be a requirement to release it on the 0.11 branch, so if there's something that requires breakage, it will have to wait.

http types at the center, hyper types as the facade

If this can be made to work, this sounds like the best. As you pointed out, it gets more testing of the http crate sooner. This could mean that people using a Service with http types could pay zero cost. The original types in hyper can get a #[deprecated] attribute, but people's code will still build using them. Internally, a compat layer could just convert things, like when making a hyper::Request, the internal http::Method, http::Uri, etc would be converted into hyper::Method etc.

@srijs
Copy link
Contributor Author

srijs commented Sep 13, 2017

@seanmonstar okay, so after an evening of experimentation, this is where I'm at:

I've explored the "http types at the core, hyper types as facade" approach. However, there is a major issue: The http crate as it stands uses associated constants, a feature that Rust 1.15 (which is configured as a test in travis) does not yet support.

So we have either the choice of raising the minimum supported version of Rust to something higher that supports associated constants (would this be considered a breaking change?), or we accept that this approach is not feasible. The conversion-methods only approach on the other hand is feasible in any case since we can hide the methods behind a feature flag, therefore making depending on http opt-in.

I have a working implementation of hyper::Uri wrapping http::Uri in my PR (#1319) in case you want to have a look, and I was able to delete a good portion of the URI parsing and extracting logic by doing that (~350 lines, see commit 9d44b62).

Please let me know how you'd like to proceed.

@seanmonstar
Copy link
Member

So we have either the choice of raising the minimum supported version of Rust to something higher that supports associated constants (would this be considered a breaking change?)

Yes, I personally find that a breaking change, when I can control it. (grumble grumble about dependencies not feeling the same, and suddenly hyper's minimum version is forced to change...)

So, it probably couldn't have a dependency on the http crate in 0.11, at least by default. I think I'm fine with a cargo feature requiring a newer compiler version. So, that would mean that by default, hyper couldn't wrap the http types, which is unfortunate.

With a feature flag turned on, that seems fair. So then there's the issue of which side should have the performance cost: default, or when using the new shiny http types.

  • It would probably feel bad if you were using hyper and its own types, and added a dependency which enabled http compat, and suddenly your server were a little slower. It shouldn't be a huge loss, but it might be surprising, especially as the user didn't directly opt-in.
  • At the same time, we want to encourage using the new types. If it's known that turning on compat and using http types is slower, people may not want to do it. Again, it shouldn't be a whole lot slower, so maybe that's fine, as people could opt-in for the API compatibility with the growing ecosystem, and know that 0.12 will remove the slight performance cost.

@seanmonstar seanmonstar added E-hard Effort: hard. Likely requires a deeper understanding of how hyper's internals work. C-feature Category: feature. This is adding a new feature. labels Sep 17, 2017
@srijs
Copy link
Contributor Author

srijs commented Sep 23, 2017

Closing this since #1319 got merged.

@srijs srijs closed this as completed Sep 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature Category: feature. This is adding a new feature. E-hard Effort: hard. Likely requires a deeper understanding of how hyper's internals work.
Projects
None yet
Development

No branches or pull requests

4 participants