-
Notifications
You must be signed in to change notification settings - Fork 991
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
Conversation
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. |
I think we should remove the code for the graphql handler if we are handling it in the API server! |
@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. |
Minor changes to the global context file too.
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.
@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 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 |
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.