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

fix(fastify): Ensure global context is unique and scoped to an individual request lifetime #8569

Merged
merged 11 commits into from
Jun 24, 2023

Conversation

Josh-Walker-GM
Copy link
Collaborator

Problem
This removes the need to have DISABLE_CONTEXT_ISOLATION=1 when using the serverful serve (aka using the experimental server file).

This needs discussion as it's not my area of expertise. @dthyresson Let's discuss this when we next chat about this code.

There was mention of using some existing fastify based solution to context isolation like https://github.com/fastify/fastify-request-context. I don't know if that would cause us to have to entangle graphql with fastify which I think we are trying to avoid.

@Josh-Walker-GM Josh-Walker-GM added the release:fix This PR is a fix label Jun 8, 2023
Copy link
Contributor

Actually this makes sense … and mimics how the GraphQLHandler works. I was thinking about this but hadn’t looked into wrapping the request so async local storage could be used. It’s way better than the envar.

@dac09
Copy link
Collaborator

dac09 commented Jun 9, 2023

I think we should remove the code for the graphql handler if we are handling it in the API server!

@Josh-Walker-GM Josh-Walker-GM marked this pull request as ready for review June 14, 2023 16:28
@Josh-Walker-GM
Copy link
Collaborator Author

@jtoar DT and I paired on this we think it's good but could maybe benefit from a fresh pair of eyes if you have time.

@Josh-Walker-GM Josh-Walker-GM changed the title fix(fastify): Ensure global context is unique and scoped to an individual request lifetime fix(fastify): Ensure global context is unique and scoped to an individual request lifetime for GraphQL requests Jun 23, 2023
This is likely inefficient to have it defined in both plugins unconditionally but right now I think it ensures context safety for all API requests.
@Josh-Walker-GM Josh-Walker-GM changed the title fix(fastify): Ensure global context is unique and scoped to an individual request lifetime for GraphQL requests fix(fastify): Ensure global context is unique and scoped to an individual request lifetime Jun 23, 2023
@Josh-Walker-GM Josh-Walker-GM enabled auto-merge (squash) June 24, 2023 14:59
@Josh-Walker-GM
Copy link
Collaborator Author

@dthyresson approves this.

This PR made changes to the context isolation so k6 load testing was used to ensure that context was being properly isolated between requests. A simple graphql request was used which took a number input stored it on the context for some random time and then returned the value in the context. This was done under heavy load and no errors in the returned value were detected which means context isolation was working correctly. Similar tests with the previous DISABLE_CONTEXT_ISOLATION env var set confirmed that in that case context was not isolated between requests.

Some tests were updated to reflect the removal of the ability to disable context isolation.

Currently for the new fastify package the hook which ensures context isolation is included in both the api and graphql fastify plugins. This might not be ideal and can be refactored later. The code here is needed to assist in moving forward with realtime features and so merging this in with some future refactoring work isn't a deal breaker.

I would not consider this breaking. We preserve the setContext api and the removal of the DISABLE_CONTEXT_ISOLATION env var is likely fine given it's existence was never documented anyway.

@Josh-Walker-GM Josh-Walker-GM merged commit c5921bc into main Jun 24, 2023
28 checks passed
@Josh-Walker-GM Josh-Walker-GM deleted the jgmw-fastify-global-context branch June 24, 2023 15:46
@redwoodjs-bot redwoodjs-bot bot added this to the next-release milestone Jun 24, 2023
@jtoar jtoar modified the milestones: next-release, v6.0.0 Jun 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix This PR is a fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants