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

Asynchronous Lambda server handler? #1989

Closed
jpetitcolas opened this issue Nov 17, 2018 · 31 comments · Fixed by #5004
Closed

Asynchronous Lambda server handler? #1989

jpetitcolas opened this issue Nov 17, 2018 · 31 comments · Fixed by #5004

Comments

@jpetitcolas
Copy link

I am using Apollo Server on AWS lambda. I followed the documentation to bootstrap it:

const { ApolloServer } = require('apollo-server-lambda');

// [...]

const server = new ApolloServer({ typeDefs, resolvers });
exports.graphql = server.createHandler();

This version works just fine. Yet, I need to get a database to pass to the GraphQL context. Hence, I need an asynchrounous handler. In this case, I can't make it work simply:

export const graphql = async (event, context, callback) => {
    const db = await getDb();

    const server = new ApolloServer({
        typeDefs: myTypeDefs,
        resolvers: myResolvers,
        context: { db },
    });

    return server.createHandler()(event, context, callback);
};

It seems the lambda function is closed before the handler executes. Diving into the code and adding some console.log, it looks like the following part is causing the issue:

      graphqlLambda(async () => {
        // In a world where this `createHandler` was async, we might avoid this
        // but since we don't want to introduce a breaking change to this API
        // (by switching it to `async`), we'll leverage the
        // `GraphQLServerOptions`, which are dynamically built on each request,
        // to `await` the `promiseWillStart` which we kicked off at the top of
        // this method to ensure that it runs to completion (which is part of
        // its contract) prior to processing the request.
        await promiseWillStart;
        return this.createGraphQLServerOptions(event, context);
      })(event, context, callbackFilter);

I don't fully understand this part of the code. Any idea of what is going on? Am I missing something?

@carbonrobot
Copy link

No change is needed to apollo-server, you need to create an async function that creates the server.

const createHandler = async () => {
   const db = await getDb();
   const server = new ApolloServer({
        typeDefs: myTypeDefs,
        resolvers: myResolvers,
        context: { db },
    });
   return server.createHandler();
};

export const graphql = (event, context, callback) => {
  createHandler().then(handler => handler(event, context, callback));
};

@nbarraille
Copy link

@carbonrobot Thanks, but we've tried that and it doesn't work. As soon as it reaches const db = await getDb();, the lambda returns.

@carbonrobot
Copy link

This is the code we are using in production, albeit a bit simplified. If you remove the callback param, you should see it timing out, if not then check for an extra async on your handler.

@nbarraille
Copy link

Thanks it worked, I wasn't executing the handler properly

@abdul-jabbar01
Copy link

No change is needed to apollo-server, you need to create an async function that creates the server.

const createHandler = async () => {
   const db = await getDb();
   const server = new ApolloServer({
        typeDefs: myTypeDefs,
        resolvers: myResolvers,
        context: { db },
    });
   return server.createHandler();
};

export const graphql = (event, context, callback) => {
  createHandler().then(handler => handler(event, context, callback));
};

You saved me man. Thank you soooo much

@adikari
Copy link
Contributor

adikari commented Feb 15, 2021

in the newer node versions the signature of handler looks like:

export const handler = async (event, context) { ... }

Shouldn't we update the package to support the newer syntax? So we can do something like:

export const handler = (event, context) => { 
   const server = new ApolloServer({
        typeDefs: myTypeDefs,
        resolvers: myResolvers,
        context: { db },
    });
    
   return server.createHandler();
}

The solutions mentioned above no longer work with nodejs14 lambdas. This is breaking change.

I have gotten it working by using promisify but it's not ideal.

const { promisify } = require('util');

export const handler = (event, context) => { 
   const server = new ApolloServer({
        typeDefs: myTypeDefs,
        resolvers: myResolvers,
        context: { db },
    });
    
   const apolloHandler = promisify(server.createHandler());
   
   return apolloHandler(event, context);
}

Can we reopen this ticket?

@glasser
Copy link
Member

glasser commented Feb 23, 2021

@adikari Would it make sense to add a new method to server like server.createHandler instead? Otherwise it seems like a serious backwards-compatibility concern, right?

If so, that seems like the sort of thing that would make a reasonable PR.

@adikari
Copy link
Contributor

adikari commented Feb 24, 2021

@glasser i am not sure what you mean. This is the breaking change on aws lambda node 14 runtime. Could you provide an example?

@glasser
Copy link
Member

glasser commented Feb 24, 2021

I'm not a Lambda expert. Are you saying that the signature of the main handler is different in Lambda Node 12 vs Lambda Node 14, in that one should accept a callback and the other should return a promise? If so, it would seem that instead of changing what server.createHandler returns, we should make a new function that returns the new type of handler?

@adikari
Copy link
Contributor

adikari commented Feb 25, 2021

Correct. In node 12 you had choice to use either async handler or non async. Non async handler will accept 3 parameters, event, context and callback. Apollo create handler requires callback to be passed. If you are using async handler, you don't get the callback. In node 12 lambda you could either use async or non async handler. So, you could do similar things suggested in the thread. Please check my previous comment for example. In node 14 handler you can only use async handler, hence you don't have the callback. Unless if you use promisify from node util as I posted in my example, create handler from this package won't work.

@glasser
Copy link
Member

glasser commented Feb 25, 2021

OK, but if we change the handler to be always async, won't that break folks on the Node 12 runtime who are configured to use the non-async mode, or on older runtimes that maybe don't support async at all?

It seems like the best fix is adding a server.createAsyncHandler function that returns the new kind of handler.

Or should the handler automatically adapt based on how many arguments it receives? Sounds like it'll lead to vaguer TypeScript types but maybe it's worth the tradeoff.

@adikari
Copy link
Contributor

adikari commented Feb 26, 2021

I have looked into the implementation of create handler. We can simply do a conditional where if the callback is undefined, then assume its a async handler. There is no need to change the existing signature and it will be backwards compatible.

@adikari
Copy link
Contributor

adikari commented Feb 26, 2021

I am happy to create a PR when I can if we agree to the proposed changes.

@glasser
Copy link
Member

glasser commented Feb 26, 2021

Ah, that makes sense. Sure, send a PR! Make sure it has appropriate tests and docs.

@glasser
Copy link
Member

glasser commented Feb 26, 2021

Since async/await functions are generally easier to read, I'd encourage you to port the function to being an async function, and then having createHandler return a little wrapper which runs the async function in the proper style.

You may want to fix #3999 while you're at it.

@adikari
Copy link
Contributor

adikari commented Feb 26, 2021

Sure. I will try to out together a PR sometimes this week.

@adikari
Copy link
Contributor

adikari commented Mar 7, 2021

@glasser I have created PR for the changes.

@glasser glasser reopened this Mar 9, 2021
@glasser
Copy link
Member

glasser commented Mar 10, 2021

@adikari Hi! I think it's a lot easier to maintain this code if it's async, so I appreciated your PR and extended it to make the bulk of the Lambda-related code async rather than callback-based.

I'm pretty sure that (pending review) I'm excited to release this, but I'd like to make sure it's documented appropriately.

Specifically, you told me that the current callback-based code just doesn't work on the Node 14 runtime.

But I went in myself to test this by following this Lambda/API Gateway tutorial. I used the Node 14 runtime and wrote this handler

exports.handler = (event, context, callback) => {
    callback(null, {
        statusCode: 200,
        body: 'Hello from Lambda.',
    });
};

and it worked fine. So while I do think it's an improvement for the code to be written async, are you sure that it's required for Node 14 runtime compatibility? I just want to make sure that our release notes are accurate..

@adikari
Copy link
Contributor

adikari commented Mar 10, 2021

I recently deployed a node 14 lambda in AWS and ran into this problem and landed on this thread again and thought to revive it. I did log the callback and found it to be undefined then resorted in using promisify. I can try it again tonight and confirm it. Just to confirm you did deploy your lambda to aws and it worked fine?

@glasser
Copy link
Member

glasser commented Mar 10, 2021

Yep, I did deploy that. Maybe it is an APIGateway vs something else difference?

@adikari
Copy link
Contributor

adikari commented Mar 10, 2021

I will try this again to confirm the behaviour. May be tomorrow. I am using API Gateway lambda proxy integration. So in theory it should be same as yours.

glasser added a commit that referenced this issue Mar 11, 2021
Lambda has two ways of defining a handler: as an async Promise-returning function, and as a callback-invoking function. https://docs.aws.amazon.com/lambda/latest/dg/nodejs-handler.html The async variety was introduced with Lambda's Node 8 runtime. Apparently the callback-invoking variety was removed with their Node 14 runtime (their docs don't mention this anywhere but our users have reported this: #1989 (comment)). While AWS doesn't directly support pre-Node-8 runtimes any more, it's possible some users are using a Custom Runtime that still requires the Node 6 version, and Apollo Server still technically supports Node 6. So we would like to support both an async and a callback-invoking handler.

To implement this, I rewrote the handler to be an async function and used a little maybeCallbackify function to make it work as either type of handler. (Apollo Server 3 will drop Node 6 support, at which point we should just make this package always return an async handler. Tracking in #5018.) This simplified a lot of the logic and I was able to replace various Promise and callback-y stuff with direct code. To make this easier, I inlined the graphqlLambda function (which was the main API in Apollo Server v1 but is not exported in v2). This makes it easier to avoid bugs like #3999.

(We aren't positive if there is actually a Node 14 compatibility issue or not; see #1989 (comment). It's still a helpful refactor though.)

Original version by @adikari.
Fixes #1989

Co-authored-by: David Glasser <glasser@davidglasser.net>
@glasser
Copy link
Member

glasser commented Mar 15, 2021

@adikari I'd like to move forward with releasing this in the next day or two. Maybe I should just change the CHANGELOG to say that we've refactored it into an async function which may make it easier to wrap it inside your own async handler, but that it doesn't affect version compatibility?

@adikari
Copy link
Contributor

adikari commented Mar 15, 2021

@glasser i have been but busy last week so didn't get chance to follow up. I will do this today and get back to you.

@adikari
Copy link
Contributor

adikari commented Mar 15, 2021

@glasser I can confirm the callback is available in nodejs 14 lambda runtime. I am not sure why I got the wrong impression with my earlier investigation. Here is the screenshot with a logged callback.

image

@glasser
Copy link
Member

glasser commented Mar 16, 2021

I've published a release with this to version 2.21.2-alpha.0. My plan is to release 2.21.2 in a day or two. If you want to test that this fix works before then, try running the prerelease yourself, and provide feedback on #5037. (I have not significantly QAed my changes in real Lambda contexts, so positive feedback on #5037 saying that the release works for your app would be great! It is intended to be a largely no-op change.)

kodiakhq bot added a commit to ProjectXero/dbds that referenced this issue Mar 17, 2021
Bumps apollo-datasource from 0.7.2 to 0.7.3.

Changelog
Sourced from apollo-datasource's changelog.

CHANGELOG
The version headers in this history reflect the versions of Apollo Server itself.  Versions of other packages (e.g., those which are not actual HTTP integrations; packages not prefixed with "apollo-server", or just supporting packages) may use different versions.
🆕 Please Note!: 🆕 The @apollo/federation and @apollo/gateway packages now live in the apollographql/federation repository.

@apollo/gateway
@apollo/federation

vNEXT

The changes noted within this vNEXT section have not been released yet.  New PRs and commits which introduce changes should include an entry in this vNEXT section as part of their development.  With few exceptions, the format of the entry should follow convention (i.e., prefix with package name, use markdown backtick formatting for package names and code, suffix with a link to the change-set à la [PR #YYY](https://link/pull/YYY), etc.).  When a release is being prepared, a new header will be (manually) created below and the appropriate changes within that release will be moved into the new section.


apollo-server-core: The SIGINT and SIGTERM signal handlers installed by default (when not disabled by stopOnTerminationSignals: false) now stay active (preventing process termination) while the server shuts down, instead of letting a second signal terminate the process. The handlers still re-signal the process after this.stop() concludes. Also, if this.stop() throws, the signal handlers will now log and exit 1 instead of throwing an uncaught exception. [Issue #4931](apollographql/apollo-server#4931)
apollo-server-lambda: (UPDATE THIS MESSAGE BEFORE RELEASE; we are not sure if this actually helps nodejs14 compatibility or if it's just a nice refactor.) Support the nodejs14 runtime by changing the handler to be an async handler. (For backwards compatibility, if the handler receives a callback, it still acts like a non-async handler.) [Issue #1989](apollographql/apollo-server#1989) [PR #5004](apollographql/apollo-server#5004)

v2.21.1

apollo-server-lambda: The onHealthCheck option did not previously work. Additionally, health checks (with onHealthCheck or without) didn't work in all Lambda contexts, such as behind Custom Domains; the path check is now more flexible. [Issue #3999](apollographql/apollo-server#3999) [PR #4969](apollographql/apollo-server#4969) [Issue #4891](apollographql/apollo-server#4891) [PR #4892](apollographql/apollo-server#4892)
The debug option to new ApolloServer (which adds stack traces to errors) now affects errors that come from requests executed with server.executeOperation (and its wrapper apollo-server-testing), instead of just errors that come from requests executed over HTTP. [Issue #4107](apollographql/apollo-server#4107) [PR #4948](apollographql/apollo-server#4948)
Bump version of @apollographql/graphql-playground-html to v1.6.27 and @apollographql/graphql-playground-react to v1.7.39 to resolve incorrectly rendered CDN URL when Playground version was false-y.  [PR #4932](apollographql/apollo-server#4932) [PR #4955](apollographql/apollo-server#4955) [Issue #4937](apollographql/apollo-server#4937)

v2.21.0

Apollo Server can now be installed with graphql@15 without causing peer dependency errors or warnings. (Apollo Server has a file upload feature which was implemented as a wrapper around the graphql-upload package. We have been unable to upgrade our dependency on that package due to backwards-incompatible changes in later versions, and the version we were stuck on did not allow graphql@15 as a peer dependency. We have now switched to a fork of that old version called @apollographql/graphql-upload-8-fork that allows graphql@15.) Also bump the graphql-tools dependency from 4.0.0 to 4.0.8 for graphql@15 support. [Issue #4865](apollographql/apollo-server#4865)

v2.20.0

apollo-server: Previously, ApolloServer.stop() functioned like net.Server.close() in that it did not close idle connections or close active connections after a grace period. This meant that trying to await ApolloServer.stop() could hang indefinitely if there are open connections. Now, this method closes idle connections, and closes active connections after 10 seconds. The grace period can be adjusted by passing the new stopGracePeriodMillis option to new ApolloServer, or disabled by passing Infinity (though it will still close idle connections). Note that this only applies to the "batteries-included" ApolloServer in the apollo-server package with its own built-in Express and HTTP servers. [PR #4908](apollographql/apollo-server#4908) [Issue #4097](apollographql/apollo-server#4097)
apollo-server-core: When used with ApolloGateway, ApolloServer.stop now invokes ApolloGateway.stop. (This makes sense because ApolloServer already invokes ApolloGateway.load which is what starts the behavior stopped by ApolloGateway.stop.) Note that @apollo/gateway 0.23 will expect to be stopped in order for natural program shutdown to occur. [PR #4907](apollographql/apollo-server#4907) [Issue #4428](apollographql/apollo-server#4428)
apollo-server-core: Avoid instrumenting schemas for the old graphql-extensions library unless extensions are provided. [PR #4893](apollographql/apollo-server#4893) [Issue #4889](apollographql/apollo-server#4889)
apollo-server-plugin-response-cache@0.6.0: The shouldReadFromCache and shouldWriteToCache hooks were always documented as returning ValueOrPromise<boolean> (ie, that they could be either sync or async), but they actually only worked if they returned a bool. Now they can be either sync or async as intended. [PR #4890](apollographql/apollo-server#4890) [Issue #4886](apollographql/apollo-server#4886)
apollo-datasource-rest@0.10.0: The RESTDataSource.trace method is now protected instead of private to allow more control over logging and metrics. [PR #3940](apollographql/apollo-server#3940)

v2.19.2

apollo-server-express: types: Export ExpressContext from main module. [PR #4821](apollographql/apollo-server#4821) [Issue #3699](apollographql/apollo-server#3699)
apollo-server-env: types: The first parameter to fetch is now marked as required, as intended and in accordance with the Fetch API specification. [PR #4822](apollographql/apollo-server#4822) [Issue #4741](apollographql/apollo-server#4741)
apollo-server-core: Update graphql-tag package to latest, now with its graphql-js peerDependencies expanded to include ^15.0.0 [PR #4833](apollographql/apollo-server#4833)

v2.19.1

apollo-server-core: The debugPrintReports option to ApolloServerPluginUsageReporting now prints traces as well. [PR #4805](apollographql/apollo-server#4805)

v2.19.0

apollo-server-testing: types: Allow generic variables usage of query and mutate functions. [PR #4383](apollograpqh/apollo-server#4383)
apollo-server-express: Export the GetMiddlewareOptions type. [PR #4599](apollograpqh/apollo-server#4599)
apollo-server-lambda: Fix file uploads - ignore base64 decoding for multipart queries. [PR #4506](apollographql/apollo-server#4506)
apollo-server-core: Do not send  operation documents that cannot be executed to Apollo Studio. Instead, information about these operations will be combined into one "operation" for parse failures, one for validation failures, and one for unknown operation names.



... (truncated)


Commits

c212627 Release
See full diff in compare view




Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.


Dependabot commands and options

You can trigger Dependabot actions by commenting on this PR:

@dependabot rebase will rebase this PR
@dependabot recreate will recreate this PR, overwriting any edits that have been made to it
@dependabot merge will merge this PR after your CI passes on it
@dependabot squash and merge will squash and merge this PR after your CI passes on it
@dependabot cancel merge will cancel a previously requested merge and block automerging
@dependabot reopen will reopen this PR if it is closed
@dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
@dependabot ignore this major version will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
@dependabot ignore this minor version will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
@dependabot ignore this dependency will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
@adikari
Copy link
Contributor

adikari commented Mar 18, 2021

I have been using this in one of my projects a so far it's been fine.

@iDVB
Copy link

iDVB commented Oct 18, 2021

We're at v3.4.0 now.
Is there an official way to execute this function as async?
Not sure where things netted out here.

@glasser
Copy link
Member

glasser commented Oct 18, 2021

Yes, this was released in 2.21.2. (And since v3, the Lambda handler only returns a promise and does not support passing in a callback.)

@iDVB
Copy link

iDVB commented Oct 18, 2021

@glasser except it's defined using aws-lambda/Handler which requires a callback.

@glasser
Copy link
Member

glasser commented Oct 19, 2021

Yeah, honestly I think the folks who put together the DefinitelyTyped package here made a bad choice: one should be allowed to write a modern async function that doesn't take a callback and have it typecheck. There's an open PR to switch away from that type (or possibly to improve the Lambda package we depend on to have more accurate types) but it doesn't currently build; it's been a couple months since I reviewed it so maybe somebody else should give it a chance. #5593

@parodoTS
Copy link

Maybe this is helpfull: Async context

 const server = new ApolloServer({
        typeDefs: myTypeDefs,
        resolvers: myResolvers,
        context: async () => ({
          db: await getDb(),
        })
 });
exports.graphql = server.createHandler();

context can be async as said in Apollo Docs

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants