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

Upgrade to support hyper 0.11 #402

Open
jolhoeft opened this Issue Jun 13, 2017 · 7 comments

Comments

Projects
None yet
2 participants
@jolhoeft
Member

jolhoeft commented Jun 13, 2017

Just announced - http://seanmonstar.com/post/161786147642/hyper-v011

This introduces some breaking changes, so may take a bit of time on a separate branch.

@jolhoeft

This comment has been minimized.

Show comment
Hide comment
@jolhoeft

jolhoeft Jun 14, 2017

Member

Quite a number of compile errors with a simple upgrade. Mostly unresolved imports, and types now private to hyper.

Member

jolhoeft commented Jun 14, 2017

Quite a number of compile errors with a simple upgrade. Mostly unresolved imports, and types now private to hyper.

@jolhoeft

This comment has been minimized.

Show comment
Hide comment
@jolhoeft

jolhoeft Oct 31, 2017

Member

I've started working on this. There are a fair number of API changes, so I expect we will want support the 0.10 train a bit, as I expect we will need to break our API in turn.

For the curious, the tip of my fork, which doesn't yet build - https://github.com/jolhoeft/nickel.rs/tree/async_402

Member

jolhoeft commented Oct 31, 2017

I've started working on this. There are a fair number of API changes, so I expect we will want support the 0.10 train a bit, as I expect we will need to break our API in turn.

For the curious, the tip of my fork, which doesn't yet build - https://github.com/jolhoeft/nickel.rs/tree/async_402

@jolhoeft jolhoeft self-assigned this Oct 31, 2017

@jolhoeft

This comment has been minimized.

Show comment
Hide comment
@jolhoeft

jolhoeft Nov 6, 2017

Member

The upgrade is going to require some breaking API changes. One I am working on now is how values are handled in body_parser.rs, which converts the request body to a String, Json, or Param struct. Currently it returns those values directly, but that is not feasible with futures. I am considering two approaches:

  1. Change them to return Box<Future<Item=String, Error=NickelError>>, etc. This will prevent code using nickel 0.10.x from compiling, and require everything to be migrated over right away.
  2. Create a new body_transformer.rs that returns the values above, and have body_parser.rs use Future::wait() to synchronize and preserve the old behavior. Since synchronizing will significantly hurt performance, body_parser.rs will need to be deprecated and eventually removed, but this will ease migrating from nickel 0.10 to 0.11.

Any thoughts or preferences?

Member

jolhoeft commented Nov 6, 2017

The upgrade is going to require some breaking API changes. One I am working on now is how values are handled in body_parser.rs, which converts the request body to a String, Json, or Param struct. Currently it returns those values directly, but that is not feasible with futures. I am considering two approaches:

  1. Change them to return Box<Future<Item=String, Error=NickelError>>, etc. This will prevent code using nickel 0.10.x from compiling, and require everything to be migrated over right away.
  2. Create a new body_transformer.rs that returns the values above, and have body_parser.rs use Future::wait() to synchronize and preserve the old behavior. Since synchronizing will significantly hurt performance, body_parser.rs will need to be deprecated and eventually removed, but this will ease migrating from nickel 0.10 to 0.11.

Any thoughts or preferences?

@jolhoeft

This comment has been minimized.

Show comment
Hide comment
@jolhoeft

jolhoeft Nov 7, 2017

Member

I've currently gone for option 2 on my personal branch. All that remains to to fix response handling, and update examples. I will probably wait until most or all of the examples are working before creating a pull request, unless someone would like to see it sooner.

Member

jolhoeft commented Nov 7, 2017

I've currently gone for option 2 on my personal branch. All that remains to to fix response handling, and update examples. I will probably wait until most or all of the examples are working before creating a pull request, unless someone would like to see it sooner.

@jolhoeft

This comment has been minimized.

Show comment
Hide comment
@jolhoeft

jolhoeft Nov 11, 2017

Member

Pull request #410 gets hello_world to work. So far it is pretty rough. More comments there.

Member

jolhoeft commented Nov 11, 2017

Pull request #410 gets hello_world to work. So far it is pretty rough. More comments there.

@nacardin

This comment has been minimized.

Show comment
Hide comment
@nacardin

nacardin Nov 15, 2017

Hey, I had worked on this a while back, but didn't have time to finish. I just cleaned it up a bit and put in #413, hope you can use some of my work if it is helpful.

nacardin commented Nov 15, 2017

Hey, I had worked on this a while back, but didn't have time to finish. I just cleaned it up a bit and put in #413, hope you can use some of my work if it is helpful.

@jolhoeft

This comment has been minimized.

Show comment
Hide comment
@jolhoeft

jolhoeft Nov 17, 2017

Member

Thanks for the PR. I've been looking over it as I can find time. We took two different approaches to handling the futures. I pushed the future in the Response, and let/make the middleware writers deal with it, while you unwrap the future and provide the contents to the middleware writer.

I like how your approach simplifies things for the middleware for a large majority of use cases. Most of the time, when there is a request body, the middleware needs the whole body before it can do much, so hiding the future is useful. I've been trying to figure out how to adapt this approach, but have run into a problem with the rest of the use cases.

Large request bodies, for example a video file upload, need to be fully uploaded and stored in a memory before the middleware can handle them. In that case though we would like to handle the stream as the upload proceeds and avoid large memory allocations.

At the moment my plan is to provide convenience methods to ease dealing with futures in the simple case (body_transformer.rs in my PR is a start on this). I will be keeping you approach in mind, though because I like the simpler API. Perhaps some outside the future hook for the streaming data case.

Member

jolhoeft commented Nov 17, 2017

Thanks for the PR. I've been looking over it as I can find time. We took two different approaches to handling the futures. I pushed the future in the Response, and let/make the middleware writers deal with it, while you unwrap the future and provide the contents to the middleware writer.

I like how your approach simplifies things for the middleware for a large majority of use cases. Most of the time, when there is a request body, the middleware needs the whole body before it can do much, so hiding the future is useful. I've been trying to figure out how to adapt this approach, but have run into a problem with the rest of the use cases.

Large request bodies, for example a video file upload, need to be fully uploaded and stored in a memory before the middleware can handle them. In that case though we would like to handle the stream as the upload proceeds and avoid large memory allocations.

At the moment my plan is to provide convenience methods to ease dealing with futures in the simple case (body_transformer.rs in my PR is a start on this). I will be keeping you approach in mind, though because I like the simpler API. Perhaps some outside the future hook for the streaming data case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment