-
Notifications
You must be signed in to change notification settings - Fork 348
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
Lambda with Async/Await #111
Conversation
Wrote a draft of the Handler that supports the |
Minor update: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=7ada5aa87c5a5a2c9d597ab7feeeaad2. Still thinking this through. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks super awesome and I can't wait to see what comes of this. Many problems I have today are immediately solved, in particular error type handling and the ability to use ?
.
I'm hesitant to expose and apigw/alb interface in the default crate. It increases the surface area and crate weight where for cases where it's not needed. In lambda it always seem to benefit users to pack light. Lifting to a sibling crate makes more sense to me.
/// Lambda function configuration from the local environment variables. | ||
/// Includes information such as the function name, memory allocation, | ||
/// version, and log streams. | ||
pub env_config: Config, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't seen this info expose this way. It benefits users who move around lambdas using different runtimes to work with types of roughly the same shape.
https://docs.aws.amazon.com/lambda/latest/dg/python-context-object.html
https://docs.aws.amazon.com/lambda/latest/dg/nodejs-prog-model-context.html
https://docs.aws.amazon.com/lambda/latest/dg/java-context-object.html
https://docs.aws.amazon.com/lambda/latest/dg/go-programming-model-context.html
if env_config is an impl detail you could keep this pub(crate)
and expose LambdaCtx
methods that provide a more std interface to the context object. What makes that awkward and would give me pause is that I like having the ability to construct these api types in unit tests so I don't know what the constructor for LambdaCtx
would look like LambdaCtx::from_config(Config::default())
??? may be worth some though before making it pub
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, for sure—@sapessi suggested throwing the environment config throw into here, but I think customers keen on getting that environment config should be able to do so themselves.
Broadly, I expect to reduce how many fields are pub
, this one included.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've thought about this for a while now. We want to make sure our runtime is friendly to Rust developers by following the standard language semantics. However, in this case these "APIs" are owned by the Lambda service and they have set a convention with all the other runtimes of exposing the config values as a property in the context object. I don't think we should break that.
I'm coming around to that. Unfortunately, I'm sure of a way that allows both:
If had to pick one or the other, I'd prioritize allowing customers to use |
I accidentally changed a prior commit and couldn't find a way to undo it, hence the force push. Sorry! |
Another possible take: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=5b370e1dd9ecd9c87a4c15ce50be954a I'm coming around to the idea that we'll need specialization to tackle both |
The last high-level question we should answer is whether the http handler type should be bundled in this crate or not. Thoughts @softprops, @davidbarsky? |
I think it should be opt in to keep the surface area small to enable cases where you want to distribute light artifacts. Preferably as another crate for discoverability or behind a cargo feature flag. Feature flags in practice make for harder to maintain systems for ci build matrix combos. Feature flags tend to be less user friendly for those coming from non rust backgrounds but are familiar with the concept of npm and jar dependencies. We are already asking users to bundle the runtime as part of the application which most other runtimes don't. I would consider it a disservice to be asked to bundle more than what I actually need to run a simple function on top of that. My ci system turnaround time also appreciates giving less source for rustc to compile! If we do bundle HTTP abstractions in core, it begs other questions like: why aren't we bundling abstractions for every other triggerable event? Which makes the case for those that what to build small artifacts even harder obtain. Not including it now means we can move forward today with all of the other concrete problems this branch solves. I'm a fan of this because my biggest pain point has been the current error api. |
Thanks for writing that up, @softprops. I'll try to respond point by point.
The thing about introducing an
In this lambda function, API, I'd like to be able to support
T<U>
where
T: !for<'de> serde::Derserialize,
U: for<'de> serde::Derserialize` That API requires the usage of negative trait bounds, which will not land on stable Rust this year, and possibly several other advanced trait features that depend on rust-lang/chalk landing in the stable compiler. Given that A specialized event type will be bundled as part of the core Lambda crate if and only if the following hold true:
At the moment, only
I don't want to block release of the error API on deciding whether we want to support the |
@davidbarsky these were helpful responses. I just want to make I understand with some concrete examples.
For apigw events, does that mean we'd end up with something like I feel like the latter is more ergonomic and possible with todays layered approach where the interface exposed wraps an internal delegation to the to core interface. Something worth considering is what if instead of a single proc macro that requires knowledge of dealing all possible event types to dispatch logic on, there could be specialized proc macros that dealt with specific cases If the api did look like
I could chip in here! Do you feel like that's enough of a breaking change on its own to bump to |
This is partly why in the current version of the runtime we have a core handler that simply deals with
I see this as a good reason for us to provide an official AWS crate to support the event type. That does not mean it needs to be bundled with the core runtime.
Agree. I'd really like to support
The proc macro is a nice simplification for users but I would not make a change to the core APIs we are not happy with for the sake of maintaining the proc macro - as @softprops said, we can easily have the separate http crate define its own macro.
Yes, if you have some spare time it would be super-helpful @softprops. |
I agree! I would prefer today's approach, which would be pub enum Body {
/// An empty body
Empty,
/// A body containing string data
Text(String),
/// A body containing binary data
Binary(Vec<u8>),
} I'll also cover how I expect (de)serialization would work from API Gateway's JSON types. Given a JSON blob like this (from https://docs.aws.amazon.com/apigateway/latest/developerguide/api-gateway-create-api-as-simple-proxy-for-lambda.html):
The I'll also note that as I was writing the above paragraph, I realized that we might not want to introduce a handler like: pub trait Handler<A, B>
where
A: for<'de> Deserialize<'de>,
B: Serialize
{
/// redacted implementation
} I think we might want something closer to: pub trait Handler<A, B> {
/// redacted implementation
} ....and have dedicated handlers that add
I don't think we should to make
I'm increasingly coming around to that idea. I don't think I ever spelled this out on GitHub, but I was thinking that the procedural macro would dispatch to a JSON-based or HTTP-based runner depending on whether One thing I'll mention is that I'd like to avoid dedicated
Please do so! I'm not 100% sure it requires a breaking change, but if it does, what's why we have semver! I apologize for not making my intent more clear! I think I'm in agreement with you with what interface we ought expose, but we are disagreeing on how we can expose that interface. |
Thanks for the extra clarifications @davidbarsky. I feel much more aligned now! I can take a stab at the error api backport as early as next weekend. |
starting to look into back the error api from this branch to 0.2 and noticed today we have #[derive(Serialize)]
pub struct ErrorResponse {
/// The error message generated by the application.
#[serde(rename = "errorMessage")]
pub error_message: String,
/// The error type for Lambda. Normally, this value is populated using the
/// `error_type()` method from the `LambdaErrorExt` trait.
#[serde(rename = "errorType")]
pub error_type: String,
/// The stack trace for the exception as vector of strings. In the framework,
/// this value is automatically populated using the `backtrace` crate.
#[serde(rename = "stackTrace")]
pub stack_trace: Option<Vec<String>>,
} this branch has #[derive(Serialize, Deserialize, Debug, PartialEq)]
pub struct ErrorReport {
/// The type of the error passed to the Lambda APIs.
pub name: String,
/// The [std::fmt::Display] output of the error.
pub err: String,
} Two questions
|
Yep! That's an oversight on my part.
We should probably support stack traces behind a (on-by-default) feature flag until rust-lang/rust#60852 lands in the stable release channel, which ought be August 15th. |
- Switch to alpha tokio/hyper releases, as runtime is not working with recent releases of futures. - Remove error hook; rely on stabilized std::any::type_name instead. - update UI tests for tokio
Is this going to supersede #106? |
What's the context around the glibc note and running rust about? |
/// tracks the invocation within the Lambda control plane. The request ID is used to specify completion | ||
/// of a given invocation. | ||
#[derive(Debug, Clone, PartialEq)] | ||
pub struct RequestId(pub String); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these wrapper types ever get used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. They can be removed, I think.
I love how far this has come, seeing how much was simplified, and how much you were able to remove. I'm wondering it would do the project well to just merge this ship an alpha and iterate. |
A lot of issues crop when applications using this project don't target
Thank you so much!
Maybe? Of the open items, I think the stream conversion, the notice about targeting only The other open items can be resolved in subsequent releases, IMO. |
Good stuff I can't wait! |
@@ -1,3 +1,4 @@ | |||
edition = "2018" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't know this was a thing but heads up, rustfmt picks up the edition from your crate if invoked via cargo so you might not need this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, it normally should, but I think rust-analyzer didn't pick it up, but I think that was fixed.
this is amazing @davidbarsky! it has been really interesting to follow along with this PR and I can't wait for the final result, great job! |
@bryantbiggs thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few comments
- ubuntu-latest | ||
- macOS-latest | ||
rust: | ||
- 1.40.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why fix to 1.40 instead of testing against stable? Do we plan to keep adding older versions here to ensure we maintain compatibility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It depends: do we intend to have an minimum supported rust version?
.github/workflows/build.yml
Outdated
env: | ||
TARGET: ${{ matrix.target }} | ||
continue-on-error: ${{ matrix.allow_failure }} | ||
fmt: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clippy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think we should add Clippy.
} | ||
} | ||
} | ||
_ => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not give them an option to accept a context? I know we have the lambda::context()
function but it feels less idiomatic than receiving the content as part of the call since the context is technically related to the invocation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, as part of the macro invocation? Good point, I think that can be a decent option.
...but it feels less idiomatic than receiving the content as part of the call since the context is technically related to the invocation.
On a somewhat related note: to sum up my thoughts on this matter—now that I've had a few months to think about it—I've seen three types of APIs in Rust for "context":
- The thread local approach. Both
std
,async-std
, and in the coming future, Tokio, take this approach. For ambient context that you need some of the time, this is the approach that seems to be preferred. - The "put it in an extensions typemap". This is what Tide and
http
do, but it does complicate trait bounds a bit and it requires every invocation to have an associated typemap. I don't think that's the correct approach here, especially since we're not in control of generating typed bindings for events. Plus, this would be too much churn IMO. - Pass the context on every invocation. This is what we do in
tracing-subscriber
's Layer (an interface for building your owntracing
exporter, among other things), but the context there is different: you need it for nearly every method to determine your current position within an execution graph.
My mental model of Lambda makes me think this approach fits best, but we can simulate how other languages supported by Lambda do it through macros.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thread local approach makes sense for all of the function configuration that is stored in the environment. Would you use the same approach for values that are specific to an invocation such as the timeout - I mean the values passed by the runtime APIs in the response headers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just wanted to offer an outside perspective - I've worked extensively with lambda in a number of languages over the past couple of years and they all offer (at minimum) the standard function signature of fn handler(event, context):
. When I first saw this in rust where context was exported to a macro, it seemed a bit odd (IMO). Are there any benefits to diverging from this norm of packing the event context into the function signature vs a macro?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thread local approach makes sense for all of the function configuration that is stored in the environment. Would you use the same approach for values that are specific to an invocation such as the timeout - I mean the values passed by the runtime APIs in the response headers?
Yep. You only care about that information some of the time, so the timeout would be placed into the lambda::context()
. Its usage isn't driven on a per instantiation-basis, but on a as-needed basis.
Are there any benefits to diverging from this norm of packing the event context into the function signature vs a macro?
I think the big disagreement between @sapessi and I that we haven't decided which grain we should go against in designing this API—Rust's or other Lambdas. I believe we should opt for aligning with Rust's grain, while Stefano has argued (and still might!) for aligning with Lambda's grain. I believe this for a few reasons:
- Rust is sufficiently different from those other languages that opting for a (slightly, in fairness) unidiomatic Rust API is a reduction in customer experience for those coming to this project from other Rust projects. I think this difference is most acutely noticed in Rust since Rust does not support optional arguments like all other officially supported languages on Lambda (see Wishlist: functions with keyword args, optional args, and/or variable-arity argument (varargs) lists rust-lang/rfcs#323 for details).
- This reduces the (already small) effort needed to bridge to other ecosystems like
tower::Service
orhttp_service::HttpService
. The pain might be most noticeable when working with generics, but this will likely be small. - For those coming from other languages to this project, I expect they won't be implementing a
Handler
by hand—they'll be using the#[lambda]
macro, which can simulate variadic arguments like other languages. If someone is implementing aHandler
by hand, I expect that the individual would already be an intermediate or advanced Rust user who already has some expectations about grabbing context from a thread local.
} | ||
} | ||
} | ||
_ => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see someone who just wants to schedule a task through CloudWatch and is not interested in any input. We can simply drop the event and ctx and start their code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solid point, I'll add that.
lambda/src/client.rs
Outdated
next_event(req).await | ||
} else if path.ends_with("response") { | ||
complete_event(req).await | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should support the init fail method too. Errors in the "construction" of a handler method should be sent to the init error endpoint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, yes—I dropped this for the initial pass. Thanks for the reminder.
let rsp = NextEventResponse { | ||
request_id: "8476a536-e9f4-11e8-9739-2dfe598c3fcd", | ||
deadline: 1542409706888, | ||
arn: "arn:aws:lambda:us-east-2:123456789012:function:custom-runtime", | ||
trace_id: "Root=1-5bef4de7-ad49b0e87f6ef6c87fc2e700;Parent=9a9197af755a6419", | ||
body: serde_json::to_vec(&body)?, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uh?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This lets us test the handler + client in memory. I'd like to replace these hard-coded values with something randomly generated, perhaps something in the style of: https://github.com/hyperium/http/blob/master/tests/header_map_fuzz.rs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thought so, thanks. Perhaps we should change the function name to make it clear that it's used for testing, or move it to the tests module.
type Fut = Fut; | ||
fn call(&mut self, req: A) -> Self::Fut { | ||
// we pass along the context here | ||
(self.f)(req) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The doc at the top of the file says that I can optionally receive a ctx. I'm probably missing the function but where is the call method that passes the ctx?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There isn't one—it's an ambient "global", local only to the Future. The context is set in the Executor::run
method.
.method(Method::POST) | ||
.uri(uri) | ||
.header("lambda-runtime-function-error-type", "unhandled") | ||
.body(Body::empty())?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this also be: serde_json::to_vec(&self.diagnostic)?;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thank you for the catch.
//! return a `Result<B, E>`, where `B` implements [`serde::Serializable`]. `E` is | ||
//! any type that implements `Into<Box<dyn std::error::Error + Send + Sync + 'static>>`. | ||
//! | ||
//! Optionally, the `#[lambda]` annotated function can accept an argument |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove this then?
Ok(()) | ||
} | ||
|
||
/// Runs the lambda function almost entirely in-memory. This is meant for testing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To keep a smaller surface area, if this is intended for tests, thoughts on scoping it to #[cfg(test)]
?
lambda/src/lib.rs
Outdated
} | ||
|
||
#[pin_project::pin_project] | ||
struct WithTaskLocal<F> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming suggestion, take it it leave it. WithLambdaCtx
is really what this is isn't it?
lambda/src/lib.rs
Outdated
} | ||
|
||
#[pin_project::pin_project] | ||
struct WithTaskLocal<F> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming suggestion, take it or leave it. WithLambdaCtx
describes what this is a little more clearly.
This PR just landed in Rusoto. I can't wait for Rust to support async AWS on all fronts, so thank you everyone for all of your work! |
Looking forward to seeing this merged! |
Thanks everyone, I just async-ified my project using this. A couple notes:
This seems to break error locations?? As in
Maybe for the |
I ran into this issue yesterday as well. |
I think—but I am not sure—that this is a limitation of proc macro hygiene in Rust at the moment. I think we might need to drop the |
This PR has been open for a while now and it's time to rip off the bandage—I'm going to merge this PR as-is and start cutting beta releases. Customers have been using this branch without issue. We'll be making a few breaking changes to those betas, but they'll be relatively minor and well-documented in the changelog. |
# Conflicts: # .github/workflows/build.yml # Cargo.lock # lambda-runtime-client/Cargo.toml # lambda-runtime-client/src/client.rs # lambda-runtime-client/src/error.rs # lambda-runtime-core/src/error.rs # lambda-runtime-errors-derive/tests/tests.rs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had already reviewed the code and there are no major changes here. Let's get this beta out!
This PR is a simplification of the Lambda Runtime for Rust. You can now write a Lambda function similar to the following:
These are the high-level changes:
LambdaError
, as the the stabilization ofstd::any::type_name
removed the need for a Lambda-specific error type.#[lambda]
annotation. Note that starting a futures executor is still necessary. In the above example, this is handled through the#[tokio::main]
macro.lambda::context()
.Advanced Usage
The Handler Trait
For greater control over a handler, this PR provides a
Handler
trait. It is defined as:This is not implemented in this PR yet, but I'd like to provide a blanket implementation of
tower::Service
, which would provide compatibility with Tower's rich ecosystem of middleware. In particular, this would enable the use oftracing
for rich logging and trace reporting.Without Macros
For those that prefer to not make use of the
#[lambda]
macro, a Lambda function can be started with the following code:Remaining Work
For this PR to land, there are some additional bits of work needed:
x86_64-unknown-linux-musl
target. The glibc in the Lambda execution environment is too old to run Rust executables.Executor::run
(seelambda/src/lib.rs
) to a stream so that Lambda functions can be easily unit-tested in-memory. By converting it to a stream, limiting execution viaSteamExt::take
or stream-cancel becomes much easier.http
-based Lambdas through the#[lambda]
macro. I think an attribute like#[lambda::http]
dispatching to anExecutor::run_http
method is the best approach until specialization is stable.tracing
/AWS X-Ray integration.cc: @softprops @iliana @sapessi.