Skip to content
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

Merged
merged 35 commits into from
Mar 11, 2020
Merged

Lambda with Async/Await #111

merged 35 commits into from
Mar 11, 2020

Conversation

davidbarsky
Copy link
Contributor

@davidbarsky davidbarsky commented Jun 28, 2019

This PR is a simplification of the Lambda Runtime for Rust. You can now write a Lambda function similar to the following:

use lambda::lambda;
use serde_json::Value;

type Error = Box<dyn std::error::Error + Send + Sync + 'static>;

#[lambda]
#[tokio::main]
async fn main(event: Value) -> Result<Value, Error> {
    let _ = lambda::context();
    Ok(event)
}

These are the high-level changes:

  • Async/await is the primary interface within a Lambda function. While Lambda does not support concurrent invocations in a single container (concurrency is the responsibility of Lambda-the-service), async/await does allow for concurrent calls to other services.
  • Functions no longer need to return a LambdaError, as the the stabilization of std::any::type_name removed the need for a Lambda-specific error type.
  • Lambda functions can be created and started through the #[lambda] annotation. Note that starting a futures executor is still necessary. In the above example, this is handled through the #[tokio::main] macro.
  • The execution context is not an argument to the handler. Instead, it is a task-local, which is similar to a thread local, but specific to a future instead of a thread. The task local can be fetched through free function lambda::context().

Advanced Usage

The Handler Trait

For greater control over a handler, this PR provides a Handler trait. It is defined as:

pub trait Handler<A, B> {
    /// Errors returned by this handler.
    type Err;
    /// The future response value of this handler.
    type Fut: Future<Output = Result<B, Self::Err>>;
    /// Process the incoming event and return the response asynchronously.
    fn call(&mut self, event: A) -> Self::Fut;
}

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 of tracing 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:

use lambda::handler_fn;

type Error = Box<dyn std::error::Error + Send + Sync + 'static>;

#[tokio::main]
async fn main() -> Result<(), Error> {
    let func = handler_fn(func);
    lambda::run(func).await?;
    Ok(())
}

async fn func(event: String) -> Result<String, Error> {
    Ok(event)
}

Remaining Work

For this PR to land, there are some additional bits of work needed:

  • Update documentation to show the usage of this runtime, especially some of the more advanced features.
  • Guide customers to only build against the x86_64-unknown-linux-musl target. The glibc in the Lambda execution environment is too old to run Rust executables.
  • Convert Executor::run (see lambda/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 via SteamExt::take or stream-cancel becomes much easier.
  • Provide examples (beyond this library's unit tests) of how to test a Lambda function.
  • Provide examples of using the runtime with tokio-compat and Rusoto, if Rusoto doesn't support async/await prior to this PR landing.
  • Figure out how to support http-based Lambdas through the #[lambda] macro. I think an attribute like #[lambda::http] dispatching to an Executor::run_http method is the best approach until specialization is stable.
  • Release tracing/AWS X-Ray integration.

cc: @softprops @iliana @sapessi.

@davidbarsky
Copy link
Contributor Author

Wrote a draft of the Handler that supports the http crate and Serde in a single trait: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=3e6b636e053eb59b7fef67bd29834770

@davidbarsky
Copy link
Contributor Author

Copy link
Contributor

@softprops softprops left a 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-attributes/Cargo.toml Outdated Show resolved Hide resolved
lambda-attributes/Cargo.toml Outdated Show resolved Hide resolved
lambda-attributes/Cargo.toml Outdated Show resolved Hide resolved
lambda/Cargo.toml Outdated Show resolved Hide resolved
lambda/examples/hello-with-ctx.rs Outdated Show resolved Hide resolved
lambda/src/lib.rs Outdated Show resolved Hide resolved
lambda/src/lib.rs Outdated Show resolved Hide resolved
/// 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,
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

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.

lambda/src/types.rs Outdated Show resolved Hide resolved
lambda/tests/compile-fail/no-args.stderr Outdated Show resolved Hide resolved
@davidbarsky
Copy link
Contributor Author

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.

I'm coming around to that. Unfortunately, I'm sure of a way that allows both:

  • lambda-attributes to be agnostic of ALB/API Gateway events,
  • Allow customers to use the http::{Request, Response} types directly.

If had to pick one or the other, I'd prioritize allowing customers to use http::Request and http::Response directly.

@davidbarsky
Copy link
Contributor Author

I accidentally changed a prior commit and couldn't find a way to undo it, hence the force push. Sorry!

@davidbarsky
Copy link
Contributor Author

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 A and http::Request<A> in using the trait system, but we might be able to sidestep the specialization requirement with an Into<Param>, where Param support both A and http::Request<A>.

@sapessi
Copy link
Contributor

sapessi commented Jul 9, 2019

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?

@softprops
Copy link
Contributor

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.

@davidbarsky
Copy link
Contributor Author

Thanks for writing that up, @softprops. I'll try to respond point by point.

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!

The thing about introducing an http-specific handler is that the http crate is already bundled as part of the Hyper dependency. Introducing a top-level http handler should have a minimal, if any, impact on overall builds, but I'd be happy to benchmark that to verify that hypothesis.

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.

In this lambda function, API, I'd like to be able to support A and http::Request<A>, where A: for<'de> serde::Derserialize. There are several obstacles:

  • http::Request<_> cannot and should not implement AWS-specific (e.g., API Gateway/ALB) deserialization logic.
  • While http::Request<_> might implement for<'de> serde::Derserialize in the future, it does not at the moment. This requires us to write an type constraint similar to the following pseudo-code:
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 http::{Request, Response} is the most common, non-directly (de)serializable type—appearing in about a third of all Lambda invocations—I believe that there is value in providing first-class support for this non-(de)serializable type in this runtime. We are able to approximate a similar API using the #[lambda] procedural macro, which requires some sort of coupling somewhere in this repository. If I were to propose a rule for what event types are bundled-in by default, it'd be:

A specialized event type will be bundled as part of the core Lambda crate if and only if the following hold true:

  • The idiomatic crate providing that event type cannot be described using a A: for<'de> Deserialize.
  • The trait system features required to support this are unavailable on stable Rust (for instance, if this were a Haskell runtime, I'd rely on GeneralizedNewtypeDeriving or DerivingVia for the http event type).
  • The event type is a commonly used event type, exceeding—for instance—20% of all Lambda invocations.
  • There exists no reasonable default (de)serialize implementation for the event type in question.

At the moment, only http::{Request, Response} fit that criteria. No other event type that I can think of has that property.

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.

I don't want to block release of the error API on deciding whether we want to support the http crate within the main runtime, nor do I think we should. At the very least, the error API is can be backported to the existing 0.2 API, but I haven't had the time to open a PR do so.

@softprops
Copy link
Contributor

@davidbarsky these were helpful responses. I just want to make I understand with some concrete examples.

http::Request<_> cannot and should not implement AWS-specific (e.g., API Gateway/ALB) deserialization logic.

For apigw events, does that mean we'd end up with something like http::Request<ApiGWRequest<UserDefinedBody>> vs todays http::Request<UserDefinedBody>> + RequestExt to expose ApiGw specific features like preparsed path and query parameters? The same for http::Response<ApiGWResponse<UserDefinedBody>>.

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 #[xxx::lambda]. This may remove some of the type system limitations your bumping into as well as the requirements for the core crate to have detailed knowledge of specific event types. This comes at the cost of the boiler plate that comes along with implementing attributes which may be a heavy enough tax to scrap this idea.

If the api did look like http::Request<ApiGWRequest<UserDefinedBody>> would that make other non apigw events representable as http::Request<CloudWatchEvent> and is that desirable? I can see how it would be possible but from a user and api perspective, it doesn't seem to align with any of the other runtimes' programming models. I'm wondering if the http::Response<OtherEventResponse> would muck with that event type's trigger response when serializing the response back to lambda for sync event triggers

At the very least, the error API is can be backported to the existing 0.2 API, but I haven't had the time to open a PR do so.

I could chip in here! Do you feel like that's enough of a breaking change on its own to bump to 0.3?

@sapessi
Copy link
Contributor

sapessi commented Jul 10, 2019

  • The idiomatic crate providing that event type cannot be described using a A: for<'de> Deserialize.
  • The trait system features required to support this are unavailable on stable Rust (for instance, if this were a Haskell runtime, I'd rely on GeneralizedNewtypeDeriving or DerivingVia for the http event type).
  • There exists no reasonable default (de)serialize implementation for the event type in question.

This is partly why in the current version of the runtime we have a core handler that simply deals with Vec<u8> and allows customers to implement their own marshalling/unmarshalling logic. At the moment Lambda only supports JSON events, that doesn't mean that in the future it won't support other encodings as well as streaming. Not sure I want to walk down that one-way door.

  • The event type is a commonly used event type, exceeding—for instance—20% of all Lambda invocations.

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.

For apigw events, does that mean we'd end up with something like http::Request<ApiGWRequest> vs todays http::Request> + RequestExt to expose ApiGw specific features like preparsed path and query parameters? The same for http::Response<ApiGWResponse>.

Agree. I'd really like to support http::Request<UserDefinedBody>, without going through the service-specific event type.

We are able to approximate a similar API using the #[lambda] procedural macro, which requires some sort of coupling somewhere in this repository.

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.

At the very least, the error API is can be backported to the existing 0.2 API, but I haven't had the time to open a PR do so.

I could chip in here! Do you feel like that's enough of a breaking change on its own to bump to 0.3?

Yes, if you have some spare time it would be super-helpful @softprops.

@davidbarsky
Copy link
Contributor Author

@softprops:

For apigw events, does that mean we'd end up with something like http::Request<ApiGWRequest<UserDefinedBody>> vs todays http::Request<UserDefinedBody>> + RequestExt to expose ApiGw specific features like pre-parsed path and query parameters? The same for http::Response<ApiGWResponse<UserDefinedBody>>.

I feel like the latter is more ergonomic and possible with today's layered approach where the interface exposed wraps an internal delegation to the to core interface.

I agree! I would prefer today's approach, which would be http::Request<UserDefinedBody>. However, I forgot that an A in http::Request<A> might not implement for<'de> Deserialize. I think we might need to rely on the existing Body struct instead:

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):

{
  "message":"Good evening, John of Seattle. Happy Thursday!", 
  "input":{
    "resource":"/helloworld",
    "path":"/helloworld",
    "httpMethod":"POST",
    "headers":{"Accept":"*/*",
    "content-type":"application/json",
    "day":"Thursday",
    "Host":"r275xc9bmd.execute-api.us-east-1.amazonaws.com",
    "User-Agent":"curl/7.64.0",
    "X-Amzn-Trace-Id":"Root=1-1a2b3c4d-a1b2c3d4e5f6a1b2c3d4e5f6",
    "X-Forwarded-For":"72.21.198.64",
    "X-Forwarded-Port":"443",
    "X-Forwarded-Proto":"https"},
    "multiValueHeaders":{"Accept":["*/*"],
    "content-type":["application/json"],
    "day":["Thursday"],
    "Host":["r275xc9bmd.execute-api.us-east-1.amazonaws.com"],
    "User-Agent":["curl/0.0.0"],
    "X-Amzn-Trace-Id":["Root=1-1a2b3c4d-a1b2c3d4e5f6a1b2c3d4e5f6"],
    "X-Forwarded-For":["11.22.333.44"],
    "X-Forwarded-Port":["443"],
    "X-Forwarded-Proto":["https"]},
    "queryStringParameters":{"city":"Seattle",
    "name":"John"
  },
  "multiValueQueryStringParameters":{
    "city":["Seattle"],
    "name":["John"]
  },
  "pathParameters":null,
  "stageVariables":null,
  "requestContext":{
    "resourceId":"3htbry",
    "resourcePath":"/helloworld",
    "htt* Connection #0 to host r275xc9bmd.execute-api.us-east-1.amazonaws.com left intact pMethod":"POST",
    "extendedRequestId":"a1b2c3d4e5f6g7h=",
    "requestTime":"20/Mar/2019:20:38:30 +0000",
    "path":"/test/helloworld",
    "accountId":"123456789012",
    "protocol":"HTTP/1.1",
    "stage":"test",
    "domainPrefix":"r275xc9bmd",
    "requestTimeEpoch":1553114310423,
    "requestId":"test-invoke-request",
    "identity":{
      "cognitoIdentityPoolId":null,
      "accountId":null,
      "cognitoIdentityId":null,
      "caller":null,
      "sourceIp":"test-invoke-source-ip",
      "accessKey":null,
      "cognitoAuthenticationType":null,
      "cognitoAuthenticationProvider":null,
      "userArn":null,
      "userAgent":"curl/0.0.0","user":null
    },
    "domainName":"r275xc9bmd.execute-api.us-east-1.amazonaws.com",
    "apiId":"r275xc9bmd"
  },
  "body":"{ \"time\": \"evening\" }",
  "isBase64Encoded":false
  }
}

The input blob would be placed into http::Request's Parts, with the json blob's headers object being placed into Parts::headers, the JSON method's httpMethod being placed into Parts::method, and so on. I'm papering over some details, but I hope that the gist of what I am attempting to communicate is clear.

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 A: for<'de> Deserialize<'de>/B: Serialize or TryInto<http::Request<lambda_http::Body>>/TryFrom<http::Response<lambda_http::Body>> bounds. This might allow us to break the http-specific handler into a dedicated crate and optionally re-export behind a feature flag.

If the api did look like http::Request<ApiGWRequest<UserDefinedBody>> would that make other non apigw events representable as http::Request<CloudWatchEvent> and is that desirable?

I don't think we should to make http::Request<CloudWatchEvent> representable. The only circumstance where I can think that can exist is if a customer creates a (questionable?) API Gateway/CloudWatch integration, but @sapessi should be able to correct me. If this is possible, we should ask customers "goodness, why would you do that?".

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 #[xxx::lambda]. This may remove some of the type system limitations your bumping into as well as the requirements for the core crate to have detailed knowledge of specific event types. This comes at the cost of the boiler plate that comes along with implementing attributes which may be a heavy enough tax to scrap this idea.

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 #[lambda(type = JSON)] or #[lambda(type = HTTP)] is provided (note: please feel free to quibble with specific syntax. It's nowhere near final).

One thing I'll mention is that I'd like to avoid dedicated lambda-attributes and lambda-http-attribute crates to distinguish between JSON and HTTP-based events. For this reason, I'm in favor of relying on cargo's feature flags to support this functionality despite that cargo's feature flags being harder to test & support in CI and it being a somewhat unfamiliar concept to npm/Maven users.

I could chip in here! Do you feel like that's enough of a breaking change on its own to bump to 0.3?

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.

@softprops
Copy link
Contributor

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.

@softprops
Copy link
Contributor

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

  1. according to the runtime docs there should be errorType and errorMessage fields. Should new ErrorReport api be using those field names?

  2. I noticed stackTrace is absent from the new api, presumably because its not something that comes for free in std lib which is fine. I just wanted to bring it up in case I was missing anything.

@davidbarsky
Copy link
Contributor Author

  1. According to the runtime docs there should be errorType and errorMessage fields. Should new ErrorReport api be using those field names?

Yep! That's an oversight on my part.

  1. I noticed stackTrace is absent from the new api, presumably because its not something that comes for free in std lib which is fine. I just wanted to bring it up in case I was missing anything.

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.

David Barsky added 2 commits August 14, 2019 18:05
- 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
@lvicentesanchez
Copy link

Is this going to supersede #106?

@softprops
Copy link
Contributor

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);
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@softprops
Copy link
Contributor

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.

@davidbarsky
Copy link
Contributor Author

@softprops

What's the context around the glibc note and running rust about?

A lot of issues crop when applications using this project don't target x86_64-unknown-linux-musl. I'm not sure I've ever seen this project successfully be used by applications when they target x86_64-unknown-linux-gnu.

I love how far this has come, seeing how much was simplified, and how much you were able to remove.

Thank you so much!

I'm wondering it would do the project well to just merge this ship an alpha and iterate.

Maybe? Of the open items, I think the stream conversion, the notice about targeting only x86_64-unknown-linux-musl, and examples using tokio-compat are the critical blockers to an alpha release. I'd like to get the stream conversion in this PR because it'd reduce churn in subsequent releases and the latter two are the primary impediments to using this runtime in production.

The other open items can be resolved in subsequent releases, IMO.

@softprops
Copy link
Contributor

Good stuff I can't wait!

@@ -1,3 +1,4 @@
edition = "2018"
Copy link
Contributor

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

Copy link
Contributor Author

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.

@bryantbiggs
Copy link

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!

@davidbarsky
Copy link
Contributor Author

@bryantbiggs thanks!

Copy link
Contributor

@sapessi sapessi left a 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
Copy link
Contributor

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?

Copy link
Contributor Author

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?

env:
TARGET: ${{ matrix.target }}
continue-on-error: ${{ matrix.allow_failure }}
fmt:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clippy?

Copy link
Contributor Author

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.

}
}
}
_ => {
Copy link
Contributor

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.

Copy link
Contributor Author

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":

  1. 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.
  2. 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.
  3. Pass the context on every invocation. This is what we do in tracing-subscriber's Layer (an interface for building your own tracing 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.

Copy link
Contributor

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?

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?

Copy link
Contributor Author

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:

  1. 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).
  2. This reduces the (already small) effort needed to bridge to other ecosystems like tower::Service or http_service::HttpService. The pain might be most noticeable when working with generics, but this will likely be small.
  3. 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 a Handler 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.

}
}
}
_ => {
Copy link
Contributor

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.

Copy link
Contributor Author

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.

next_event(req).await
} else if path.ends_with("response") {
complete_event(req).await
} else {
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Comment on lines +107 to +113
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)?,
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uh?

Copy link
Contributor Author

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.

Copy link
Contributor

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)
Copy link
Contributor

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?

Copy link
Contributor Author

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())?;
Copy link
Contributor

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)?;

Copy link
Contributor Author

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
Copy link
Contributor

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.
Copy link
Contributor

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)]?

}

#[pin_project::pin_project]
struct WithTaskLocal<F> {
Copy link
Contributor

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?

}

#[pin_project::pin_project]
struct WithTaskLocal<F> {
Copy link
Contributor

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.

@vultix
Copy link

vultix commented Jan 21, 2020

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!

@davidbarsky davidbarsky mentioned this pull request Jan 26, 2020
2 tasks
@ayax79
Copy link

ayax79 commented Feb 7, 2020

Looking forward to seeing this merged!

@valpackett
Copy link

Thanks everyone, I just async-ified my project using this. A couple notes:

#[lambda] #[tokio::main]

This seems to break error locations?? As in rustc doesn't show where in that annotated function errors are if they exist. (I'm on rustc 1.42.0-nightly (c2d141df5 2020-01-24)) Only using #[tokio::main] with lambda::run is fine.

The glibc in the Lambda execution environment is too old to run Rust executables

Maybe for the rustup official distributions of Rust, but building with the Rust 1.39.0 that's in CentOS7/EPEL (and using shared libs from there) still works fine :)

@iliana
Copy link

iliana commented Feb 10, 2020

This seems to break error locations?? As in rustc doesn't show where in that annotated function errors are if they exist. (I'm on rustc 1.42.0-nightly (c2d141df5 2020-01-24)) Only using #[tokio::main] with lambda::run is fine.

I ran into this issue yesterday as well.

@davidbarsky
Copy link
Contributor Author

This seems to break error locations?? As in rustc doesn't show where in that annotated function errors are if they exist. (I'm on rustc 1.42.0-nightly (c2d141df5 2020-01-24)) Only using #[tokio::main] with lambda::run is fine.

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 tokio::main to recover better error messages.

@davidbarsky
Copy link
Contributor Author

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
Copy link
Contributor

@sapessi sapessi left a 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!

@davidbarsky davidbarsky merged commit dd4cb4a into master Mar 11, 2020
@ajpauwels ajpauwels mentioned this pull request Apr 12, 2020
@nmoutschen nmoutschen deleted the async-await branch February 1, 2022 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.