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

Port to Hyper's tokio branch #501

Open
untitaker opened this Issue Nov 15, 2016 · 11 comments

Comments

Projects
None yet
6 participants
@untitaker
Member

untitaker commented Nov 15, 2016

Rough plan:

  • Add a new trait AsyncHandler with a method handle_async. There is a impl<H: Handler> AsyncHandler for H that just delegates an invocation of Handler::handle(self, ...) to a threadpool. (where is the threadpool stored? Request extensions?). The idea is that users can still impl synchronous handlers using Handler, but drop down to AsyncHandler if they really care about performance.

    • Important: Implement AsyncHandler for functions, etc
  • All middlewares become async. Synchronous middlewares are not supported. AroundMiddleware takes Box<AsyncHandler> and returns Box<AsyncHandler>

  • Everything else in Iron goes async as well.

  • The response object will have to change somehow, probably. Hopefully response modifiers can abstract the changes away.

@Hoverbear

This comment has been minimized.

Show comment
Hide comment
@Hoverbear

Hoverbear Nov 15, 2016

Member

This will be a big job and introduce significant breaking changes.

Member

Hoverbear commented Nov 15, 2016

This will be a big job and introduce significant breaking changes.

@ernestas-poskus

This comment has been minimized.

Show comment
Hide comment
@ernestas-poskus

ernestas-poskus Nov 16, 2016

If you plan to make middlewares async it might brake expected behavior where authorization or alike middleware must go first.

ernestas-poskus commented Nov 16, 2016

If you plan to make middlewares async it might brake expected behavior where authorization or alike middleware must go first.

@untitaker

This comment has been minimized.

Show comment
Hide comment
@untitaker

untitaker Nov 16, 2016

Member

Middlewares would still be applied in order.

Member

untitaker commented Nov 16, 2016

Middlewares would still be applied in order.

@Hoverbear

This comment has been minimized.

Show comment
Hide comment
@Hoverbear

Hoverbear Nov 16, 2016

Member

Right. By async we mean the middlewares themselves should support being async, so they should not block, for example. They would still be done in order.

Member

Hoverbear commented Nov 16, 2016

Right. By async we mean the middlewares themselves should support being async, so they should not block, for example. They would still be done in order.

@hayd

This comment has been minimized.

Show comment
Hide comment
@hayd

hayd Jan 18, 2017

Hyper's tokio branch is now in master. hyperium/hyper@2d2d557

hayd commented Jan 18, 2017

Hyper's tokio branch is now in master. hyperium/hyper@2d2d557

@untitaker

This comment has been minimized.

Show comment
Hide comment
@untitaker

untitaker Feb 19, 2017

Member

#523 updates to async Hyper, for now just using a threadpool

Member

untitaker commented Feb 19, 2017

#523 updates to async Hyper, for now just using a threadpool

@MJDSys

This comment has been minimized.

Show comment
Hide comment
@MJDSys

MJDSys Mar 27, 2017

Contributor

I've started digging into the meat of this issue (Async Iron on Async Hyper, based off of #523). However, I'm running some lifetime issues.

Basically, when my async_handle function returns, I can't have references to either the handler or the request hanging around, as the response isn't constructed until later. For requests, I've tried stepping around this by moving in a Request instead of passing a &mut Request, and producing a Request as part of the output. I'm not super happy about this though, as it leaks the details of the Chain into async_handle's interface. Also there is now an ugly tuple to return. The trait is currently:

/// `AsyncHandler`s are responsible for handling all requests by returning a future that yields the actual response.
pub trait AsyncHandler: Send + 'static {
    /// Produce a `Future` that handles the given request that produces an appropriate response or error.
    fn async_handle(&self, Request) -> BoxIronFuture<(Request, Response)>;
}

// where BoxIronFuture is a helper type defined as:
pub type BoxIronFuture<T> = Box<futures::Future<Item=T, Error=IronError>>;

However, if anything in &self is needed, it needs to be cloned into the future. This is a problem implementing Chain, as the Vecs of middleware are needed to decide how to proceed, but I can't keep a reference to them. I've currently fixed it by avoiding BoxFuture, which requires the Future to be Send (since we are not currently sending the future across threads, which I'm not sure is a valid assumption?). I then store a Vec<Rc<*Middleware>> instead of Vec<Box<*Middleware>>, but I'm not super happy about that. If I use BoxFuture, Vec<Arc<*Middleware>> isn't enough, as *Middleware isn't Sync. I'm not sure if it's a problem to require *Middleware to be Sync though.

I tried making the &self in async_handle 'static, but due to Hyper not allowing its handler to be 'static, that doesn't work. So it seems I'm stuck with an Rc.

I'm thinking the Vec<Rc<*>> needs to be a Rc<Vec<Box<*>>> to avoid some Box<Future<>> in Chain, but that's an optimization with a separate discussion.

Also reem/rust-plugin#14 remains a problem. For now, I've removed the Extensible implementation and used a SendMap directly.

Contributor

MJDSys commented Mar 27, 2017

I've started digging into the meat of this issue (Async Iron on Async Hyper, based off of #523). However, I'm running some lifetime issues.

Basically, when my async_handle function returns, I can't have references to either the handler or the request hanging around, as the response isn't constructed until later. For requests, I've tried stepping around this by moving in a Request instead of passing a &mut Request, and producing a Request as part of the output. I'm not super happy about this though, as it leaks the details of the Chain into async_handle's interface. Also there is now an ugly tuple to return. The trait is currently:

/// `AsyncHandler`s are responsible for handling all requests by returning a future that yields the actual response.
pub trait AsyncHandler: Send + 'static {
    /// Produce a `Future` that handles the given request that produces an appropriate response or error.
    fn async_handle(&self, Request) -> BoxIronFuture<(Request, Response)>;
}

// where BoxIronFuture is a helper type defined as:
pub type BoxIronFuture<T> = Box<futures::Future<Item=T, Error=IronError>>;

However, if anything in &self is needed, it needs to be cloned into the future. This is a problem implementing Chain, as the Vecs of middleware are needed to decide how to proceed, but I can't keep a reference to them. I've currently fixed it by avoiding BoxFuture, which requires the Future to be Send (since we are not currently sending the future across threads, which I'm not sure is a valid assumption?). I then store a Vec<Rc<*Middleware>> instead of Vec<Box<*Middleware>>, but I'm not super happy about that. If I use BoxFuture, Vec<Arc<*Middleware>> isn't enough, as *Middleware isn't Sync. I'm not sure if it's a problem to require *Middleware to be Sync though.

I tried making the &self in async_handle 'static, but due to Hyper not allowing its handler to be 'static, that doesn't work. So it seems I'm stuck with an Rc.

I'm thinking the Vec<Rc<*>> needs to be a Rc<Vec<Box<*>>> to avoid some Box<Future<>> in Chain, but that's an optimization with a separate discussion.

Also reem/rust-plugin#14 remains a problem. For now, I've removed the Extensible implementation and used a SendMap directly.

@MJDSys MJDSys referenced this issue Mar 30, 2017

Open

Async handler #527

0 of 9 tasks complete
@bfrog

This comment has been minimized.

Show comment
Hide comment
@bfrog

bfrog Apr 4, 2017

Will this let me build my own listener per thread now? Its been a long time pain for me that everything using iron must be Sync+Send if shared among handlers when what I'd really like is per thread state rather than global state.

bfrog commented Apr 4, 2017

Will this let me build my own listener per thread now? Its been a long time pain for me that everything using iron must be Sync+Send if shared among handlers when what I'd really like is per thread state rather than global state.

@MJDSys

This comment has been minimized.

Show comment
Hide comment
@MJDSys

MJDSys Apr 4, 2017

Contributor

@bfrog My current implementation in #527 currently is single threaded, and the AsyncHandler's don't require Sync+Send. However, the Request does need to be Send due to needing Handler implementations needing to be run on a thread pool, and thus requiring the Request to be sendable across threads (and thus anything you put in the request's extensions needs to be Sendable).

I don't know if the Iron devs will want to change that.

Contributor

MJDSys commented Apr 4, 2017

@bfrog My current implementation in #527 currently is single threaded, and the AsyncHandler's don't require Sync+Send. However, the Request does need to be Send due to needing Handler implementations needing to be run on a thread pool, and thus requiring the Request to be sendable across threads (and thus anything you put in the request's extensions needs to be Sendable).

I don't know if the Iron devs will want to change that.

@bfrog

This comment has been minimized.

Show comment
Hide comment
@bfrog

bfrog Apr 4, 2017

Thats perfect. No I don't extend the Request's themselves, I'd like to have per thread handlers that look something like.... struct MyHandler { db_conn: Conn } where conn doesn't need to be shared among a bunch of threads. Your changes make it sound like this will work perfectly with that.

bfrog commented Apr 4, 2017

Thats perfect. No I don't extend the Request's themselves, I'd like to have per thread handlers that look something like.... struct MyHandler { db_conn: Conn } where conn doesn't need to be shared among a bunch of threads. Your changes make it sound like this will work perfectly with that.

@MJDSys

This comment has been minimized.

Show comment
Hide comment
@MJDSys

MJDSys Apr 4, 2017

Contributor

Yep, that should work fine. The only problem is that you can't do any long work in the async_handle function, as you block the IO as well. I'm looking to start using my branch for a project, and I'm looking to use tokio_postgres to talk to my database for that reason. You might also have to share that one connection across several requests, but at least you don't have to worry about threading :)

Contributor

MJDSys commented Apr 4, 2017

Yep, that should work fine. The only problem is that you can't do any long work in the async_handle function, as you block the IO as well. I'm looking to start using my branch for a project, and I'm looking to use tokio_postgres to talk to my database for that reason. You might also have to share that one connection across several requests, but at least you don't have to worry about threading :)

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