Skip to content
This repository has been archived by the owner on May 19, 2023. It is now read-only.

Make Rustful asynchronous #102

Closed
wants to merge 14 commits into from
Closed

Make Rustful asynchronous #102

wants to merge 14 commits into from

Conversation

Ogeon
Copy link
Owner

@Ogeon Ogeon commented Mar 29, 2016

Adapt the library to the new asynchronous Hyper. This will break many projects out there, but it's a necessary evil if we don't want to fire up threads for the requests. The most basic servers, that doesn't read the request body, will be almost exactly the same, but the body has to be read using a closure, since it happens after handle_request has finished. The only real problem with this, so far, is that the Context can't be moved into that closure. It has to be picked apart first. You can see this in action in some of the examples, like examples/post.rs.

A new and interesting feature is the ability to make low level, fully asynchronous handlers. I'm even considering making the Handler trait from Hyper compatible with them, which will introduce some interesting possibilities. The API for these RawHandlers is still not finished, so feel free to come with ideas and suggestions.

The current state of this PR is that it builds, the basic features works, and the examples works, but it's very incomplete. Some features are missing and some are in a temporarily broken state. It can still be used, so feel free to give it a spin, but expect dragons and missing laundry. The brave souls who would like to try it can do so by adding these lines to their Cargo.toml:

[dependencies.rustful]
git = "https://github.com/Ogeon/rustful"
branch = "async"

I will sporadically update it as I move along, and you can run cargo update to get the new stuff.

Feel free to suggest any changes or give any other kinds of feedback. I'm especially interested in opinions regarding the way the request body is treated and the handler related traits.

Closes #51.

@homu
Copy link
Collaborator

homu commented Apr 5, 2016

☔ The latest upstream changes (presumably #103) made this pull request unmergeable. Please resolve the merge conflicts.

@Ogeon
Copy link
Owner Author

Ogeon commented May 14, 2016

The most recent commit changes the server to use scoped threads by default, meaning that the routers and handlers no longer needs to be 'static. The old behavior is still available through my_server.run_detached().

@Ogeon Ogeon force-pushed the async branch 2 times, most recently from a97f5fc to 2ae5bec Compare May 20, 2016 16:27
@Ogeon Ogeon modified the milestone: 0.9.0 May 28, 2016
@NicolasLM
Copy link

I've been playing a bit with the async branch and it worked flawlessly. It's great to see a Rust HTTP framework based on asynchronous IO!

@Ogeon
Copy link
Owner Author

Ogeon commented Jun 7, 2016

Thank you, that's really nice to hear! 😄 These features has been on my wish list for a long time, so I'm also happy to be able to finally implement them.

By the way (and this is for everyone) feel free to come with any feedback regarding the API (What's good? What could be better? etc.). Especially the more low level parts. I'm almost done reimplementing the old features, so it's ok to start focusing on cosmetics and usability. The remaining features (essentially just chunked responses) should have a low impact on the overall interface.

@NicolasLM
Copy link

I'm wondering how to interact properly with synchronous code.

Most web apps frequently query databases over the network. None of the existing Rust db drivers support asynchronous IO and as far as I see, it's not going to change in the near future.

Is there a way to avoid blocking all clients during calls to a database? I was thinking about a thread pool dedicated to such blocking calls.

@Ogeon
Copy link
Owner Author

Ogeon commented Jun 8, 2016

There's no special tool for it, at the moment, but it's something that has to be dealt with. The same situation (if not worse) occur when one has to send files, and the current file sending code will just block. I can't yet say if a thread pool is the way to go or not, but it's worth considering. One possible problem I can see is the risk of clogging it with tasks, which is the same problem as with having a blocking IO server with a thread pool.

@Ogeon
Copy link
Owner Author

Ogeon commented Jun 11, 2016

A work pool of some sort is at least better than nothing, after all. Actually, you could spin up your own specialized one and then give references to it to your handlers, but it would be nice with an internal general purpose one, as well. At least for sending files.

I think I'll add one and perhaps even expose it through the Context.

@NicolasLM
Copy link

A great article on how nginx deals with this issue: https://www.nginx.com/blog/thread-pools-boost-performance-9x/

TL;DR: Works well to prevent a small percentage of requests doing blocking IO to block the event loop.

@Ogeon
Copy link
Owner Author

Ogeon commented Jun 11, 2016

There we go. It will probably need some tweaking, but it's something.

@NicolasLM
Copy link

Cool!

I gave it a show and tried to send a 10G file but there seems to be an issue with send_file:

wget -vvv -O /dev/null --limit-rate=1M 0:8080/res/data.raw
--2016-06-11 23:59:21--  http://0:8080/res/data.raw
Resolving 0 (0)... 0.0.0.0
Connecting to 0 (0)|0.0.0.0|:8080... connected.
HTTP request sent, awaiting response... 200 OK
Length: 10737418240 (10G) [application/octet-stream]
Saving to: ‘/dev/null’

/dev/null             0%[                      ]   2.77M  1.05MB/s   in 2.6s   

2016-06-11 23:59:24 (1.05 MB/s) - Connection closed at byte 2901944. Retrying.

Funny thing is, depending on the rate limit set, the probability of the stream being cut changes.

@Ogeon
Copy link
Owner Author

Ogeon commented Jun 11, 2016

That's strange... It could be some bug in the write loop that makes it abort when it shouldn't. I'll have to check that out later.

@homu
Copy link
Collaborator

homu commented Jun 16, 2016

☔ The latest upstream changes (presumably #114) made this pull request unmergeable. Please resolve the merge conflicts.

@hjr3
Copy link
Contributor

hjr3 commented Jul 18, 2016

@Ogeon let me know if you want anything help with this PR. i am excited to get the hyper async IO support.

@Ogeon
Copy link
Owner Author

Ogeon commented Jul 18, 2016

Thank you for the offer! I'm always grateful for any help, so feel free to make PRs to this branch. I do, however, not have a real checklist, except for that file transfer problem. The goal for this PR is just to port everything and make it work, merge it when Hyper 0.10 is out, and then polish it in later PRs.

The main features are already ported, so I have been taking it slow recently (while having my brain stuffed with new stuff from work). What's most needed right now is testing and bug hunting.

That being said, I'm open to suggestions.

@homu
Copy link
Collaborator

homu commented Jul 28, 2016

☔ The latest upstream changes (presumably #118) made this pull request unmergeable. Please resolve the merge conflicts.

@Ogeon
Copy link
Owner Author

Ogeon commented Aug 6, 2016

I've managed to narrow the lifetime of callbacks down to the scope of the threading environment, which allows non-static references in more places, such as when reading the request body. This makes things slightly more verbose, but I think it may be worth it. It requires an explicit lifetime parameter when implementing Handler:

impl<'env> Handler<'env> for MyHandler {
    fn handle_request(context: Context, response: Response) { //... }
}

...and sometimes on the handler method or function:

impl<'env> Handler<'env> for MyHandler {
    fn handle_request<'a>(context: Context<'a, 'env>, response: Response<'env>) { //... }
}

fn my_handler<'a, 'env>(context: Context<'a, 'env>, response: Response<'env>) { //... }

...but usually only when response is passed to a Body callback, as far as I know. The error messages from the compiler, when things are wrong, are not too cryptic and gives correct suggestions. Having 'env on Handler makes it possible to have references in MyHandler that's valid for 'env and use them in callbacks:

struct MyHandler<'env> {
    a: &'env A,
    b: &'env B,
}

impl<'env> Handler<'env> for MyHandler<'env> {
    fn handle_request<'a>(context: Context<'a, 'env>, response: Response<'env>) {
        let a = self.a;
        let b = self.b;

        context.body.read_to_end(move |body| {
            response.send(a.make_response(b, body));
        })
    }
}

Let me know what you think and if you have any ideas for how it can be improved.

@hjr3
Copy link
Contributor

hjr3 commented Aug 12, 2016

@Ogeon now that futures are a thing and tokio looks to be the official choice for hyper, is there much benefit to reducing 'static references?

@Ogeon
Copy link
Owner Author

Ogeon commented Aug 12, 2016

I don't know. I didn't know of their existence when I wrote that, and I have just now started to think about it. It depends on how tokio will work, I guess, and if it allows anything non-'static. Hyper is still in a kind of design phase, so I don't think I'll take any action just yet, but it's not hard to change. I'll have to do a lot of other changes, anyway, to adapt it to the new interface.

I haven't tested promises yet, but I saw that they are required to be 'static, so there's a risk that there will be problems, but I'm not completely sure. People will at least want to use them, so it's good to keep them in mind, but I don't want to integrate them into the public API, to avoid version conflicts. I'll try to make it compatible with them, though. I think it will work out well if Response is compatible with them. Everything else can be either destructured or converted to something 'static.

That's my thoughts, off the top of my head. I think I'll play around with futures this weekend and try to wrap my head around how they work. Have you tried any of them?

@hjr3
Copy link
Contributor

hjr3 commented Aug 12, 2016

That's my thoughts, off the top of my head. I think I'll play around with futures this weekend and try to wrap my head around how they work. Have you tried any of them?

@Ogeon Not in Rust. I am setting aside some time next week to start prototyping some stuff with the futures crate and tokio. I was hoping you would educate me 😄

I will update here with any opinions once I write some code.

@Ogeon
Copy link
Owner Author

Ogeon commented Aug 12, 2016

I was hoping you would educate me 😄

I wish! 😄 I have had so many other things on my mind this past week, and so little time.

I will update here with any opinions once I write some code.

That would be great! I'll of course do the same and perhaps adjust things if necessary.

@Ogeon Ogeon modified the milestones: 0.10.0, Async Sep 24, 2016
@Ogeon
Copy link
Owner Author

Ogeon commented Oct 12, 2016

I saw that 'static isn't required by Promise anymore, so I suppose that will be less of a problem. Still, I'll hold off a bit more and wait for Hyper to progress.

@homu
Copy link
Collaborator

homu commented Apr 14, 2017

☔ The latest upstream changes (presumably #121) made this pull request unmergeable. Please resolve the merge conflicts.

@Ogeon Ogeon changed the title [WIP] Make Rustful asynchronous [Archived] Make Rustful asynchronous Apr 14, 2017
@Ogeon Ogeon changed the title [Archived] Make Rustful asynchronous Make Rustful asynchronous Apr 14, 2017
@Ogeon
Copy link
Owner Author

Ogeon commented Apr 14, 2017

This has diverged too far to continue. I'm closing it and opening another branch when it's time again.

@Ogeon Ogeon closed this Apr 14, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants