router.to_async(), simple async/.await examples #281

alsuren · 2018-10-07T21:13:22Z

In a previous iteration of this PR, I noticed in another project that I was wanting to use try!() a lot, now that my code looked like sync code, so I made a handler_try!() macro. In this version of the PR, I just use nested async blocks. Both are a bit ugly. I'm not sure which is better.

It would be really nice if we could have an impl of Handler for async functions (it would let me collapse my fn _() {Box::new(async {}.boxed(...).compat())} boilerplate into async fn _() {...}). Not the end of the world though. This pr now includes a .to_async() method on router.

The examples are a bit contrived, because they don't call anything that can fail, so the lack of a working ? operator isn't brought to the surface. It's probably worth porting at least examples/handlers/form_urlencoded before we cut a release, and decide whether there is a way to pass State around that is compatible with ? (I probably won't have time for this until Febuary). This may cause us to change the signature of .to_async().

colinbankier · 2018-10-23T11:32:54Z

@alsuren - thanks for sharing your work on this. Cool to see examples as async/await solidifies.

alsuren · 2019-11-16T12:08:28Z

I've basically rewritten this from scratch. I would say that it is ready for review now.

If you like this then ~~I will~~ it would be good to try examples/handlers/form_urlencoded next. (I probably won't have time for this until Febuary)

colinbankier · 2019-11-21T05:18:53Z

Thanks for your efforts here @alsuren 👍 .
I'm wondering if we should wait to see if #370 means we update gotham to use Futures 0.3 soon anyway, then these example don't need to bother with the compat stuff.
Nice to have some examples of how it can be done however and maybe Futures 0.3 will take longer than I'm hoping.

pksunkara · 2019-12-30T17:32:57Z

I would vote to wait until #370 is done.

alsuren · 2020-01-20T21:04:37Z

@pksunkara I rebased on top of master and simplified things to match the simplification in simple_async_handlers (sleep() can no longer fail). We probably want another set of examples that shows error handling tricks, but I will leave that up to someone else.

I have left the awkward async {...}.boxed() stuff as it is for now. I would really love to be able to define handlers as async fn sleep_handler(...) and then call route.get(...).to_async(sleep_handler) or something. If someone wants to take over this PR and implement that, I'm happy to let them.

codecov · 2020-01-20T21:05:17Z

Codecov Report

Merging #281 into master will increase coverage by 0.08%.
The diff coverage is 86.66%.

@@            Coverage Diff             @@
##           master     #281      +/-   ##
==========================================
+ Coverage   79.51%   79.59%   +0.08%     
==========================================
  Files         105      106       +1     
  Lines        4964     5023      +59     
==========================================
+ Hits         3947     3998      +51     
- Misses       1017     1025       +8

Impacted Files	Coverage Δ
gotham/src/handler/mod.rs	`81.81% <ø> (ø)`
...s/handlers/simple_async_handlers_await/src/main.rs	`86.20% <86.20%> (ø)`
gotham/src/router/builder/single.rs	`95.45% <100.00%> (+0.45%)`	⬆️
gotham/src/handler/assets/mod.rs	`89.05% <0.00%> (-0.05%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c99941...702e7cf. Read the comment docs.

pksunkara · 2020-01-20T22:07:30Z

examples/handlers/simple_async_handlers_await/src/main.rs

+extern crate serde;
+#[macro_use]
+extern crate serde_derive;
+extern crate tokio;


Please use 2018 edition. Don't need so many extern crate statements.

alsuren · 2020-01-20T22:08:12Z

I just pushed a POC implementation of the router.to_async() function that I've been longing for. If you like it, please tell me what I need to do in order to get it merged. If you would rather merge this PR without it, please shout and I will revert it.

pksunkara

Minor changes needed

examples/handlers/simple_async_handlers_await/Cargo.toml

alsuren · 2020-01-21T19:13:24Z

gotham/src/router/builder/single.rs

+        Fut: Future<Output = HandlerResult> + Send + 'static,
+    {
+        self.to_new_handler(move || Ok(move |s: State| handler(s).boxed()))
+    }


I have implemented this directly on the trait because it feels like the most backwards-compatible thing to do.

In practice, the only implementation of DefineSingleRoute in the gotham codebase is SingleRouteBuilder. Would you rather I implement it on SingleRouteBuilder and force all external implementors of DefineSingleRoute to implement it?

Also, can someone look extra-hard at the marker traits that I've used on H and Fut, and make sure they're all appropriate? (I copied some of them from .to(), and I'm not sure they're all needed)

I'm not sure. In a world where you don't care about backwards compatibility, which would you go for? The versioning model we are on allows us to break things, and we can provide documentation for migration if it's complicated.

The traits seem ok to me (when looking at a diff). I feel like Copy and 'static might be unnecessary...?

Ideally, the .to should map to (State, Response<Body>) (normal fn), Pin<Box<HandlerFuture>> (future fn) and HandlerResult (async fn).

We can take it one step forward and disallow the mapping to (State, Response<Body>) and replace it with HandlerResult for consistency.

If both future fn and async fn could not be supported at the same time and we are willing to break, then we should give priority to async fn and create a new to_future or something for the future fn.

@whitfin that sounds like the right question to ask. Good thinking. In an ideal world, I would probably think about making this trait private or something (I think that a recent compiler release that allows traits to be marked nonexhaustive, so they can't be implemented by external people?). In the short term, "program to the interface" seems to be the pattern that's being used here, so refactoring the implementation down into SingleRouteBuilder sounds like the thing to do. I will add this to the checklist, and do it once I have all of the other questions worked out.

@pksunkara: (I think that this is a separate discussion) as it stands in my PR (using your terminology) I can pass a "future fn" to .to_async(), because Pin<Box> implements Future<Output = HandlerResult>, but I can't pass "normal fn".

When I wrote this, I didn't think it's possible to create a single function that can accept both an impl Trait and a struct. I seem to remember, rustc complaining that my blanket impl HandlerFuture for impl Future might conflict with my impl HandlerFuture for (State, Response<Body>) if someone else implements Future for (State, Response<Body>)... or something. You can get away with it for Pin<Box<HandlerFuture>> because in that case your impl is over two struct types that the compiler can guarantee won't overlap. This was last year though, so I might be misremembering, or the compiler might have become more lenient since then.

I've been mulling it over in my head over the last couple of days, and I think that it might be possible to impl Future for (State, Response<Body>), and get on with our lives. Do you think that would work?

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=19c7034bdda2cb834f574c041788111f

error[E0117]: only traits defined in the current crate can be implemented for arbitrary types --> src/lib.rs:7:1 | 7 | impl Future for (State, String) { | ^^^^^^^^^^^^^^^^--------------- | | | | | this is not defined in the current crate because tuples are always foreign | impl doesn't use only types from inside the current crate | = note: define and implement a trait or new type instead

:(

How about making the return object HandlerResult for normal fn?

same deal: HandlerResult is just a Result that's generic over tuples, so it' also foreign.

9 | impl Future for HandlerResult { | ^^^^^^^^^^^^^^^^------------- | | | | | `std::result::Result` is not defined in the current crate | impl doesn't use only types from inside the current crate

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=1e88ddbdf314a06d0871b2b7fe10ee82

I think that we need a separate .to() for tuples vs .to_async() for futures. It might be that we should call them .to_sync() and .to() though.

The traits seem ok to me (when looking at a diff). I feel like Copy and 'static might be unnecessary...?

I had a quick play about and I couldn't see a way to get rid of them.

The handler callback is FnOnce + Copy. This is a bit like Fn, but a bit easier to implement with closures or something? If I remove the + Copy then I end up with a handler function that can only be run once.

+ 'static is required by .to_new_handler(). It feels a bit excessive at first glance, but I think it's fine: the routes we define normally live for the entire duration of the program, and are normally implemented as top-level fns. I haven't really dug into whether there is a more appropriate lifetime than this, so I'm just going to keep what's there.

nyarly

I'm inclined to merge this unless there's objection before 5/21/20

pksunkara · 2020-05-20T10:33:46Z

@nyarly This is not really an objection but just some bikeshedding. Since 0.5.0 is a breaking change anyway, why not map .to for async and .to_non_async for the normal ones? I don't mind either way.

msrd0 · 2020-05-20T10:49:52Z

@pksunkara I dislike the name change. At least I've used closures (.to(|state| ...)) method many times in the past. This would become a lot more elaborate .to_non_async(|state| ...) even if I meant to return a boxed future since async closures are not stable yet.

msrd0 · 2020-05-20T14:00:25Z

examples/handlers/simple_async_handlers/README.md

@@ -24,6 +24,9 @@ find yourself doing lots of CPU/memory intensive operations on the web server,
 then futures are probably not going to help your performance, and you might be
 better off spawning a new thread per request.

+If you came here looking for an example that uses async/.await, please read
+[Async Request Handlers (.await version)](../simple_async_handlers).


I believe this link is incorrect as it points to itself if I'm not mistaken

Good spot. Thanks.

nyarly

Merge on 5/21/20

alsuren force-pushed the async-await branch from 89ade35 to 700b700 Compare October 7, 2018 21:18

alsuren force-pushed the async-await branch from 700b700 to 23d2eef Compare November 4, 2018 11:37

alsuren force-pushed the async-await branch from af7ffc6 to 04ca92e Compare November 16, 2019 11:32

alsuren changed the title ~~WIP async/await example~~ async/.await example Nov 16, 2019

alsuren force-pushed the async-await branch from 04ca92e to 309c89f Compare November 16, 2019 11:54

alsuren mentioned this pull request Nov 21, 2019

Migrate Gotham to std Futures to support Async/Await #370

Merged

12 tasks

alsuren force-pushed the async-await branch from 5467394 to 231aea2 Compare January 20, 2020 20:41

pksunkara reviewed Jan 20, 2020

View reviewed changes

pksunkara suggested changes Jan 21, 2020

View reviewed changes

examples/handlers/simple_async_handlers_await/Cargo.toml Outdated Show resolved Hide resolved

examples/handlers/simple_async_handlers_await/Cargo.toml Outdated Show resolved Hide resolved

alsuren added 9 commits January 21, 2020 18:39

copy simple_async_handlers as a base

9bbf9da

.await version of the simple_async_handlers example

bf2bca8

make docstrings easier to read

7d7258a

rustfmt fix

a960ef0

make it compile after rebase

9cba316

simplify example now we're not using ?

122015a

POC router.to_async()

60f8831

Import macros explicitly like it's 2020

6ef8666

review comments

6294e75

alsuren force-pushed the async-await branch from 9fcf2df to 6294e75 Compare January 21, 2020 18:43

alsuren added 2 commits January 21, 2020 18:45

remove irrelevant bit of readme

3940d49

add example to to_async, and fix type soup

448fc39

alsuren commented Jan 21, 2020

View reviewed changes

make deps more similar to combinator version

51630b6

alsuren changed the title ~~async/.await example~~ router.to_async() and some simple async/.await examples Jan 21, 2020

alsuren changed the title ~~router.to_async() and some simple async/.await examples~~ router.to_async(), simple async/.await examples Jan 21, 2020

separate interface from implementation

9e8c30a

colinbankier added this to the 0.5 milestone Feb 27, 2020

nyarly previously approved these changes May 20, 2020

View reviewed changes

msrd0 reviewed May 20, 2020

View reviewed changes

fix link spotted by msrd0

702e7cf

alsuren dismissed nyarly’s stale review via 702e7cf May 20, 2020 17:54

nyarly approved these changes May 20, 2020

View reviewed changes

alsuren mentioned this pull request May 23, 2020

Allow ? in routes #431

Closed

pksunkara approved these changes Jun 3, 2020

View reviewed changes

pksunkara merged commit 885a026 into gotham-rs:master Jun 3, 2020

alsuren mentioned this pull request Mar 26, 2021

Async-closures or reference-parameterized-closures-returning-futures rust-lang/wg-async#78

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

router.to_async(), simple async/.await examples #281

router.to_async(), simple async/.await examples #281

alsuren commented Oct 7, 2018 •

edited

Loading

colinbankier commented Oct 23, 2018

alsuren commented Nov 16, 2019 •

edited

Loading

colinbankier commented Nov 21, 2019

pksunkara commented Dec 30, 2019

alsuren commented Jan 20, 2020

codecov bot commented Jan 20, 2020 •

edited

Loading

pksunkara Jan 20, 2020

alsuren commented Jan 20, 2020

pksunkara left a comment

alsuren Jan 21, 2020 •

edited

Loading

whitfin Feb 20, 2020

pksunkara Feb 20, 2020

pksunkara Feb 20, 2020 •

edited

Loading

alsuren Feb 22, 2020

alsuren Feb 22, 2020

pksunkara Feb 22, 2020 •

edited

Loading

alsuren Feb 22, 2020

alsuren Feb 22, 2020

nyarly left a comment

pksunkara commented May 20, 2020

msrd0 commented May 20, 2020

msrd0 May 20, 2020

alsuren May 20, 2020

nyarly left a comment

router.to_async(), simple async/.await examples #281

router.to_async(), simple async/.await examples #281

Conversation

alsuren commented Oct 7, 2018 • edited Loading

colinbankier commented Oct 23, 2018

alsuren commented Nov 16, 2019 • edited Loading

colinbankier commented Nov 21, 2019

pksunkara commented Dec 30, 2019

alsuren commented Jan 20, 2020

codecov bot commented Jan 20, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

alsuren commented Jan 20, 2020

pksunkara left a comment

Choose a reason for hiding this comment

alsuren Jan 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pksunkara Feb 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pksunkara Feb 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nyarly left a comment

Choose a reason for hiding this comment

pksunkara commented May 20, 2020

msrd0 commented May 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nyarly left a comment

Choose a reason for hiding this comment

alsuren commented Oct 7, 2018 •

edited

Loading

alsuren commented Nov 16, 2019 •

edited

Loading

codecov bot commented Jan 20, 2020 •

edited

Loading

alsuren Jan 21, 2020 •

edited

Loading

pksunkara Feb 20, 2020 •

edited

Loading

pksunkara Feb 22, 2020 •

edited

Loading