-
-
Notifications
You must be signed in to change notification settings - Fork 532
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
Do not block graphql_transport_ws
operations while creating context or validating a single request operation
#2829
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2829 +/- ##
==========================================
- Coverage 96.79% 96.53% -0.27%
==========================================
Files 517 517
Lines 33517 33598 +81
Branches 5570 5573 +3
==========================================
- Hits 32444 32433 -11
- Misses 845 926 +81
- Partials 228 239 +11 |
33caef4
to
2008261
Compare
Thanks for adding the Here's a preview of the changelog: Operations over Here's the tweet text:
|
bd252c1
to
a8f064c
Compare
a8f064c
to
c6044e5
Compare
c6044e5
to
94e545e
Compare
Thanks for updating @kristjanvalur, still validating the changes 👍 |
94e545e
to
716a64b
Compare
716a64b
to
bc69cdd
Compare
4377cfb
to
1117caf
Compare
eb34b88
to
f2632d0
Compare
5ee6010
to
d17a4d4
Compare
7058795
to
03689c0
Compare
03689c0
to
a1d0695
Compare
PR is updated but I cannot see from the codecov that there is any missing coverage for my changes. |
This PR needs to be completely refactored because of the upstream changes. |
Hi Kristján, thanks for bringing this up again! During a brief discussion of this PR at PyconIT earlier this year, I realized that the strawberry core devs I talked to generally perceive Based on the assumption that these operations are lightweight, we were less concerned about |
Sounds good. I'll leave the context/root as they are and rework this to just do the validation. |
It appears that validation now happens in the task ,because the |
Actually, my tests as written for the old PR do indicate a discrepancy in validation handling for queries vs subscriptions. |
By the way, speaking of "context" being created once for each WS connection, I want to draw your attention to this bug: #1754 |
Now both
Context
creation and validation of an operation (subscripton or single-result operation) are performedon the worker task, freeing up the Websocket for other requests.
Description
Previously both creating the context, and validating the request, were done on the same task which handles
incoming messages for the Websocket. Both of these operations are async and can potentially take a long time.
Now they are performed on the worker background
Task
which is created to handle the operation.We add tests to make sure that long operations in context and validation don't block the websocket connection.
Types of Changes
Issues Fixed or Closed by This PR
Checklist