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 nested execution contexts and contexts created in a separate async execution within the same stack #2459

Closed
wants to merge 3 commits into from

Conversation

enisdenjo
Copy link
Collaborator

@enisdenjo enisdenjo commented Nov 1, 2023

Closes #2227

Often found behaviour, especially when using @envelop/graphql-modules:

  1. Create the operation controller in one plugin hook and use its context in another hook (different async execution, but same stack; for ex. onContextBuilding creates, onExecute uses)
  2. Create the operation controller and use its context in a nested async execution
  3. Create nested operation controllers. If the user does invokes createOperationController, they are creating a nested context since the plugin itself [already created an operation controller]](https://github.com/n1ru4l/envelop/blob/main/packages/plugins/graphql-modules/src/index.ts#L19C1-L22)

Note that creating an operation controller creates an execution context.

The current implementation of execution context handler has 2 work modes: using AsyncLocalStorage.enterWith (Node v14+) or async_hooks.createHook (fallback).

When using AsyncLocalStorage.enterWith, 1. and 2. will work because it binds to the stack. However, 3. will create wonky behaviour (probably related to nodejs/node#45848 and nodejs/help#3041). Node doesn't recommend using AsyncLocalStorage.enterWith because if its "magical" behaviour, reduced visibility and harder debugging (see comments from the related issues above). Additionally, even though AsyncLocalStorage.enterWith was introduced with Node v14, it's still considered unstable and Node's team doesn't want to promote it to stable because of the aforementioned reasons (see nodejs/node#35286). AsyncLocalStorage was introduced in #2395.

On the other hand, when using async_hooks.createHook, both 1., 2. and 3. will fail. Where 1. and 2. is much more important (because of @envelop/graphql-modules). The reason it fails is because of how the createHook's init is implemented - it only considers nested async executions, not parallel ones within the same stack.

TODO

  • Should nested operation controllers even be allowed?

Copy link

changeset-bot bot commented Nov 1, 2023

⚠️ No Changeset found

Latest commit: 9dee25b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

github-actions bot commented Nov 1, 2023

💻 Website Preview

The latest changes are available as preview in: https://74339b92.graphql-modules.pages.dev

@darkbasic
Copy link
Contributor

If the user does invokes createOperationController, they are creating a nested context since the plugin itself already created an operation controller

It's the other way around: in the context building function you manually create the operation controller before @envelop/graphql-modules has a chance to create its own. If it was the other way around the user wouldn't have to invoke createOperationController to access the injector from the context building function. I don't like having to manually create an operation controller so it would be nice if @envelop/graphql-modules could be hooked up before the context creation, for example by wrapping the context creation function.

As annoying as it may be this is unfortunately not the culprit of the issue leading to Cannot read properties of undefined (reading 'getModuleContext'), which is due to the async context getting lost when using AsyncLocalStorage.enterWith.
Since it works with Apollo I guessed it must be something in yoga/envelop which ends up losing the async context and that's why I suggested the problem might be outside graphql-modules (unless we decide to part with enterWith).

@enisdenjo enisdenjo deleted the exec-context-stuff branch November 6, 2023 11:28
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.

Cannot read properties of undefined (reading 'getModuleContext') when using @ExecutionContext()
2 participants