Plugin event callbacks should be async

[jsshapiro]

All currently defined event callbacks for plugins are synchronous. This means, for example, that one cannot wrap operations in a transaction if the database connection is in turn async.

The fix is to add await at all of the callback invocations in processGraphQLRequest in apollo-service-core/requestPipeline.ts, which is already an async function. await has no effect if called on non-async functions, so this should be completely backwards compatible.

If there is interest in adopting this change, I'll be happy to create a mergeable repo.

In abstract, the transaction could be started in a context factory. The problem is that there's no clean way to close the transaction connection on completion in that approach.

[glasser]

Can you name an example of a synchronous event callback? I'm pretty sure most of them are async (or well, they're an odd "you can make them sync or async" thing).

[jsshapiro]

https://github.com/apollographql/apollo-server/blob/main/packages/apollo-server-core/src/requestPipeline.ts#L404
https://github.com/apollographql/apollo-server/blob/main/packages/apollo-server-core/src/requestPipeline.ts#L494
https://github.com/apollographql/apollo-server/blob/main/packages/apollo-server-core/src/requestPipeline.ts#L501
https://github.com/apollographql/apollo-server/blob/main/packages/apollo-server-core/src/requestPipeline.ts#L538

There are several places where invokeHookSync is used to invoke a plugin hook. See line 404 as an example. All of these uses appear to be calling execution start/end plugin callbacks, which especially need to run before/after the actual execution. Unless there is something special about the execution event callbacks that I'm not taking into account, I think the calls to invokeHookSync should be replaced with awaited calls to invokeHookAsync throughout. That change is fully compatible with existing uses of synchronous handler functions.

There are also places (the other three links above, but the list is not complete) where it sure looks like event callback functions are being invoked directly off of the extensionStack without await. These callbacks are invoked elsewhere using invokeHookAsync, so it would appear that they are supposed to be async-enabled callbacks. If so (unless I'm failing to understand something), I think those procedure calls should be awaited.

On a less critical note, but important for developers, it would be helpful if the documentation identified which plugin callbacks are and are not able to be async. Especially so when the current behavior isn't consistent about it. This entry says nothing about the execution callback function needing to be synchronous, and the entire discussion on that page doesn't say anything one way or the other about whether callbacks are async-safe.

[glasser]

You're right, there are some sync callbacks. Note that the extension stack is an old interface (which didn't have any sort of cross-request state) which is fully superseded by the plugin API and which will be removed in AS3. But yes, you are right that a number of the plugin events are still sync (especially the ones that have matching "end" callbacks).

Making these all async could be a nice AS3 (or later) change. I agree that the docs should be better here. The TypeScript types are correct, at least (the async ones return ValueOrPromise).

[jsshapiro]

So I get all of what you’re saying. Problem is: this means middleware cannot correctly wrap execution in current Apollo-server. Deferring that to AS3 seems unfortunate, as I’d bet there’s a fair bit of ignorantly broken code in the field on this one.

…

On Tue, Mar 9, 2021 at 2:12 PM David Glasser ***@***.***> wrote: You're right, there are some sync callbacks. Note that the extension stack is an old interface (which didn't have any sort of cross-request state) which is fully superseded by the plugin API and which will be removed in AS3. But yes, you are right that a number of the plugin events are still sync (especially the ones that have matching "end" callbacks). Making these all async could be a nice AS3 (or later) change. I agree that the docs should be better here. The TypeScript types are correct, at least (the async ones return ValueOrPromise). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#4999 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEJX3CVKT6QGZGJHPCBX75LTC2FKVANCNFSM4YXIINYQ> .

[glasser]

AS3 will hopefully be in the not too distant future!

[abernix]

I don't know what the original motivation was but the choosing of sync hooks appears to be somewhat intentional, since the hooks (even in the new Plugin aPI) which are still synchronous are wrapping underling functions calls into graphql (e.g., parse and validate) which are themselves synchronous. Just mentioning this though, and I certainly wouldn't challenge an opportunity for there to be a future that changes that!

However, for today (in case it's useful!) this was an example solution that was drawn up some time back by @queerviolet that worked to solve a particular desire to wrap execution in a transaction by one of our users — note there is also an imperative example below the first block example:

https://gist.github.com/queerviolet/41c01c6b7b5198f688c0b6e10523e735

Note though this is just wrapping execution, though as a matter of principle, I'm not sure if you actually want to be wrapping your parsing and validation invocations in a database transaction.

Anyhow, hope this is helpful!

[jsshapiro]

Thank you, @aberniz.

Even when a hook wraps something synchronous, that doesn't mean that the hook function wants to be synchronous. We may need to do async things around the parse function (like I/O) even if the parse function itself is synchronous.

If you read it more closely, you'll see that the code you cite illustrates the problem, not the solution.

First, that code isn't building a transaction; it's building a promise for a transaction. If that approach is used, every single use of the transaction connection in the resolvers must await the transaction. This is necessary exactly (and only) because it can't be awaited from inside a non-async plugin callback.

Second, neither variant is executing any cleanup code at the end of the transaction, which was my original goal.

Third, if something goes wrong setting up the transaction, the exception context will be bizarre.

Have another look at that code and ask yourself what happens if the plugin acquires and releases a database connection from a pool for each operation (which it definitely should be doing). Ask yourself where it is possible to put the required connection release that won't leave you with an unresolved promise when an exception occurs during release (and yes, that can happen). Note that in up-coming versions of Node, unresolved promises will be treated as server-fatal errors.

Then ask yourself how you would make that approach work if a mutation transaction aborts because of a read or write conflict and needs to be re-tried. That can't be solved with the plugin/callback approach. It truly requires wrapping the execute function so that the execute function can be called again - something that is relatively easy to do in graphql-js but does not seem to be possible in ApolloServer. Though perhaps I just can't figure out how.

@glasser: Could you please give a moment's thought to my transaction retry example? Unless I'm missing something, that can't be done with the plugin approach. Do you want another bug opened to expose the execute function similar to graphql-js customExecuteFn?

[glasser]

For what it's worth, the gateway argument does allow you to override execution. (I have personally been skeptical of our choice to not just use the plugin interface for that purpose, though I'm not planning to change that decision.)

[glasser]

See #4051 and #4050.