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

Support for asynchronous lambda invocation with integration type AWS #5898

Merged
merged 2 commits into from Mar 21, 2019

Conversation

@snurmine
Copy link
Contributor

commented Mar 4, 2019

What did you implement:

Closes #4862

How did you implement it:

How can we verify it:

service: env-test
provider:
  name: aws
  runtime: nodejs6.10

functions:
   lambda:
    handler: index.handler
    events:
      - http:
          path: users
          method: post
          async: true

Running sls package should generate template that has

 "RequestParameters": {
     "integration.request.header.X-Amz-Invocation-Type": "'Event'"
 }

Didn't test with real AWS API GW. Expected beheviour would that it return HTTP 202.

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

@eahefnawy

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

Thanks for jumping on this @snurmine ... From the checklist above, it seems that this PR is ready for review, however it seems that the documention is missing. Could you add that?

Didn't test with real AWS API GW. Expected beheviour would that it return HTTP 202.

Could you test it with real AWS API GW? :) ... Unit tests are great, but we can't merge anything that is untested manually.

@eahefnawy eahefnawy self-assigned this Mar 5, 2019

@eahefnawy eahefnawy self-requested a review Mar 5, 2019

@eahefnawy eahefnawy added this to the 1.40.0 milestone Mar 5, 2019

@eahefnawy eahefnawy added this to In progress in Serverless via automation Mar 5, 2019

@snurmine

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

Agree this should be tested with real AWS API GW although I took hints from the issue.

@snurmine

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

Tested with

service: myService 

provider:
  name: aws
  runtime: nodejs8.10

functions:
  hello:
    handler: handler.hello
    events:
      - http:
          path: users/create
          method: get
          async: true
'use strict';

module.exports.hello = async (event) => {

 await ti(1000)

  return {
    statusCode: 400,
    body: JSON.stringify({
      message: 'Go Serverless v1.0! Your function executed successfully!',
      input: event,
    }),
  };

};

function ti(millis) {
  return new Promise((resolve, reject) => {
    setTimeout(() => {
      resolve('');
    }, millis)
  })
}

It seems that it return before sleep is done, because it respons less than 1s.
if commented that integration: aws_async it takes more than 1s.
Response code by default seems to be 200. It might require additional configuration in the template to get 202.

@eahefnawy

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

Thanks for the updates @snurmine and for adding the docs. I'm gonna test this and get back to you! 🙌

@eahefnawy

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

@snurmine works great. One more feedback though, don't you think it'd be better UX if we config it like this:

functions:
  hello:
    handler: handler.hello
    events:
      - http:
          path: users/create
          method: get
          async: true # default is false

rather than:

functions:
  hello:
    handler: handler.hello
    events:
      - http:
          path: users/create
          method: get
          integration: aws_async

This would give a nicer experience imo like we're doing with cors. It'd also match how you're validating it. What you think?

@snurmine

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

Could be, since integration refers to aws apigw integration types(mock, lambda etc.). Only thing is that, maybe the feature is not so helpful. I wonder if something like adding custom headers in general, would be more beneficial. Then adding giving custom header and setting integration type to "lambda"(or what allows customs mapping) would give same effect.

Serverless automation moved this from In progress to Needs review Mar 20, 2019

@pmuens
Copy link
Member

left a comment

Great job @snurmine 👍

I just looked through this and it looks good so far. The only thing I don't understand right now is that we're providing 2 ways to configure it (at least as far as I understand it). There's async: true and integration: aws_async.

Is it possible that we reduce this to only one config option? I'd vote for async: true.

Do we need both? It seems like integration: aws_async is not always available (e.g. when using an existing integration type) and users should pick async: true in that case.

Because of that async: true seems to be more flexible 🤔

@snurmine

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

Thank you. I removed type aws_async. The async: true was internal helper that was not promoted in the template level. Just using async: true is cleaner.

@snurmine snurmine force-pushed the snurmine:issues/4862 branch from 4514bee to 6d82631 Mar 20, 2019

@pmuens pmuens assigned pmuens and unassigned eahefnawy Mar 21, 2019

@pmuens
pmuens approved these changes Mar 21, 2019
Copy link
Member

left a comment

Thanks for updating this @snurmine 👍

I just tested it and it works great! LTGM :shipit:

Serverless automation moved this from Needs review to Reviewer approved Mar 21, 2019

@pmuens pmuens merged commit 31f5f2f into serverless:master Mar 21, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Serverless automation moved this from Reviewer approved to Done Mar 21, 2019

@horike37

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

Great! This is exactly what I just wanted 👍
Thanks for implementing @snurmine

@bsdkurt

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

Glad to see someone took this on and finished it!

I just wanted to point out that @HyperBrain mentioned this in comment #4862 (comment):

As the asynchronous invocation is only valid with AWS type integrations, it could be encoded into a new integration type: AWS_ASYNC. Having a separate async property could make people believe that it is independent from the integration and should do something in any case.

Is async: true missleading for other integration types?

@snurmine

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

Cloudformation might fail for integration types that don't support adding RequestParameters, but I didn't test. Could add some validation to definitely invalid combination, but it might be easier to let AWS validate the template. Now the integration type refers only to Cloudformation field Integration.Type and thus it might be easier to understand and existence of async just adds that extra header.

@louspringer

This comment has been minimized.

Copy link

commented Apr 23, 2019

Increasing the timeout over 30 seconds on an async lambda throws a warning like
Serverless: WARNING: Function post-ec2 has [a] timeout of 120 seconds, however, it's attached to API Gateway so it's automatically limited to 30 seconds.

Shouldn't the warning be suppressed for async lambdas?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.