Handlers can be async #68

illicitonion · 2019-01-06T11:03:51Z

Fixes #14

Handlers now return an IntoFuture (which Result implements), so that async handlers can be written. In particular, this is useful for using rusoto, because it means that its Futures will be executed on the tokio runtime.

This is mostly backwards compatible, because Result implements IntoFuture, but it adds a Send + 'static bound to a few types (notably Handler, Event) so should probably be considered a breaking change.

It also stops us calling wait() everywhere except the top-level, but it tries to make minimally invasive changes. At some point I'd be tempted to change all of the inline panic! calls to be Future error returns, and handle them at the top-level, rather than panic!ing all over the place.

Each commit is useful to separately review, as they are separated out building blocks. Sorry that the example I added is so contrived...!

By submitting this pull request

[✔] I confirm that my contribution is made under the terms of the Apache 2.0 license.
[✔] I confirm that I've made a best effort attempt to update all relevant documentation.

Result implements IntoFuture, so this is backwards compatible, assuming Handler (and its Event type) are Send and 'static, which they almost certainly always are.

sapessi · 2019-01-07T17:22:41Z

Hi @illicitonion, thanks for the PR. We also planned to make the whole runtime async eventually. We were holding off for now because we have a pretty significant refactor in progress in #63. I'll @davidbarsky chime in here since he owns the async RFC.

davidbarsky · 2019-01-08T04:48:17Z

Thanks for cutting this CR! I think this is an excellent start from my cursory overview. I was also looking at replacing the panics with a top-level panic handler, but fell down a rabbit hole with Tower integration.

davidbarsky · 2019-01-09T17:06:08Z

README.md


 `Handler` provides a default implementation that enables you to provide a Rust closure or function pointer to the `lambda!()` macro.

+If your handler is synchronous, you can just return a `Result` from it; if your handler is asynchronous, you can return a `Future` from it.


Thanks for these docs! Small phrasing nit—I'd prefer avoiding using “just” in documentation. I'd instead emphasize that due to the IntoFuture implementation on Result, most synchronous handlers won't need to make any code changes.

davidbarsky · 2019-01-09T17:06:47Z

lambda-http/src/lib.rs


 /// Functions serving as ALB and API Gateway handlers must conform to this type.
-pub trait Handler<R> {
+pub trait Handler<R>: Send {


Is Send not inferred on Handler?

Because Handler is a trait, there's no way for the type system to know that an implementation of it doesn't have some non-Send fields. We can either add the bound here to require that implementors are Send, or we can add a + Send bound in each of the places that requires Handler be Send, but doing the latter would require adding the bound to several core functions, and would lead to more confusing error messages if you had a non-Send trait.

davidbarsky · 2019-01-09T17:12:27Z

lambda-http/src/lib.rs

 /// # Panics
 /// The function panics if the Lambda environment variables are not set.
-pub fn start<R>(f: impl Handler<R>, runtime: Option<TokioRuntime>)
+pub fn start<R>(f: impl Handler<R> + 'static, runtime: Option<TokioRuntime>)


Two things:

I'm not completely sure that impl Handler needs to be &'static. I wonder if start<R> can be refactored to take an impl Future instead of an impl Handler<R>.

If we're able to get rid of all .waits() inside the client, we can remove the optional TokioRuntime parameter.

I'll hold off on looking at this until #63 merges, if that's ok

davidbarsky · 2019-01-09T17:13:57Z

lambda-runtime-client/src/client.rs

    /// Creates a new instance of the Runtime APIclient SDK. The http client has timeouts disabled and
    /// will always send a `Connection: keep-alive` header.
-    pub fn new(endpoint: String, runtime: Option<Runtime>) -> Result<Self, ApiError> {
+    pub fn new(endpoint: String, task_executor: TaskExecutor) -> Result<Self, ApiError> {


The signature can be changed to just return Self—we're not returning ApiError anywhere now (nice job that!)

davidbarsky · 2019-01-09T17:19:15Z

lambda-runtime-client/src/client.rs

-                        resp.status()
-                    )));
+        let http_client = self.http_client.clone();
+        uri.into_future()


I didn't know that Uri had an IntoFuture implementation. Can I ask why you opted for this?

I think uri here is a Result and all Result types get a free IntoFuture impl. Since the return type changed, the ? needed to be removed above

Yeah; I could change this to future::done(uri) if that's more clear?

davidbarsky · 2019-01-09T17:31:35Z

lambda-runtime/src/runtime.rs

 where
-    E: serde::de::DeserializeOwned,
-    O: serde::Serialize,
+    E: serde::de::DeserializeOwned + Send + 'static,


I'm think I'm missing why the event needs to be static—aren't we taking ownership of it here?

We're taking ownership of the instance of E - the 'static here is stating that that instance's lifetime doesn't depend on any other (non-static) lifetimes; otherwise the E may have a reference to some other data in it which could be dropped, which would mean the E instance was no longer valid. It's a bound on whether E itself can have borrows, rather than on how long the E instance actually lives.

davidbarsky · 2019-01-09T17:34:40Z

lambda-runtime/src/runtime.rs

            runtime_client: client,
            settings: config,
-            handler: f,
+            handler: Arc::new(Mutex::new(f)),


I'm not the biggest fan of having a mutex here. Can you elaborate why it's necessary?

Because run takes a &mut self, we're only allowed to call run from one thread at a time. This Mutex is how we guarantee that multiple threads don't call run.

If we wanted to drop the Mutex, we would need to either:

Change run from taking &mut self to just taking &self (this would mean we could only implement it for Fn not for FnMut like we currently do). This would be easy, but an API change.

Somehow guarantee that we only have one thread which calls a Handler. This is what we were doing before (by blocking the executions); if we wanted to do this, we could probably use some kind of same thread executor, but I imagine that would be awkward for actually writing handlers, because they couldn't necessarily rely on the runtime they submit tasks to being able to do more than one thing at a time.

davidbarsky · 2019-01-09T17:35:12Z

lambda-runtime/src/runtime.rs

    F: Handler<E, O>,
-    E: serde::de::DeserializeOwned,
-    O: serde::Serialize,
+    E: serde::de::DeserializeOwned + Send + 'static,


See earlier comments about the 'static bound w.r.t. ownership.

davidbarsky · 2019-01-09T17:37:24Z

lambda-runtime/src/runtime.rs

+        let settings = self.settings.clone();
+        let handler = self.handler.clone();
+
+        loop_fn((), move |()| {


We might have the opportunity to refactor this chunk of code into a bunch of smaller and more methods & futures.

Yeah, this could very reasonably become a series of nicely tightly scoped maps and and_thens in a follow-up change :)

davidbarsky · 2019-01-09T17:59:27Z

lambda-runtime/src/runtime.rs

+        settings: FunctionSettings,
+    ) -> impl Future<Item = (E, Context), Error = String> {
+        loop_fn(
+            (0, None),


I was thinking of using tower-retry for retries in a slightly less ad-hoc way (e.g., https://gist.github.com/seanmonstar/2469a8e544ada2d31d13c8ee54f17d48). I'll talk to the Tower folks about getting them on crates.io. I've been doing that locally with some success.

I'll happily give this a go when the crate is published! :)

Sounds good!

softprops · 2019-01-12T01:56:18Z

Futures retry exists and doesn't depend on anything but the futures crate https://mexus.gitlab.io/futures-retry/futures_retry/

saks · 2019-04-22T16:54:45Z

Any progress on this?

nappa85 · 2019-05-10T13:57:16Z

Sorry, didn't saw this PR before making mine...

davidbarsky · 2019-05-10T14:11:34Z

@nappa85 you're fine! don't worry about it.

illicitonion · 2019-05-10T20:53:03Z

Sorry, didn't saw this PR before making mine...

No worries @nappa85! I haven't had time to fix this up, and something that's done and works is definitely better than something that's part-finished! :)

softprops · 2020-05-26T20:51:32Z

I think this can be closed now. First class async support now exists in master

illicitonion added 11 commits January 6, 2019 10:42

RuntimeClient::next_event returns a Future

2f7eb74

RuntimeClient::event_response returns a Future

a5072a0

RuntimeClient::event_error returns Future

4301ffd

Drive execution from a tokio Runtime

5adedef

get_next_event uses a future loop_fn

695333c

Runtime::start is fully async

e96a013

Handler returns IntoFuture not Result

c65d0e5

Result implements IntoFuture, so this is backwards compatible, assuming Handler (and its Event type) are Send and 'static, which they almost certainly always are.

Add contrived async Handler example

2f6e599

Update README

3aa5a85

cargo fmt

f2ae75f

cargo fmt with nightly

0f253c6

davidbarsky self-requested a review January 9, 2019 16:58

davidbarsky reviewed Jan 9, 2019

View reviewed changes

Review comments

5d1de6d

illicitonion closed this May 26, 2020


		`Handler` provides a default implementation that enables you to provide a Rust closure or function pointer to the `lambda!()` macro.

		If your handler is synchronous, you can just return a `Result` from it; if your handler is asynchronous, you can return a `Future` from it.

Handlers can be async #68

Handlers can be async #68

Uh oh!

Conversation

illicitonion commented Jan 6, 2019

Uh oh!

sapessi commented Jan 7, 2019

Uh oh!

davidbarsky commented Jan 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

softprops Jan 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

softprops commented Jan 12, 2019

Uh oh!

saks commented Apr 22, 2019

Uh oh!

nappa85 commented May 10, 2019

Uh oh!

davidbarsky commented May 10, 2019

Uh oh!

illicitonion commented May 10, 2019

Uh oh!

softprops commented May 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

softprops Jan 12, 2019 •

edited

Loading