Do not block graphql_transport_ws operations while creating context or validating a single request operation

[kristjanvalur]

Now both Context creation and validation of an operation (subscripton or single-result operation) are performed
on 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

• Core
• Bugfix
• New feature
• Enhancement/optimization
• Documentation

Issues Fixed or Closed by This PR

Checklist

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• I have tested the changes and verified that they work and don't break anything (as well as I can manage).

[codecov]

Codecov Report

Attention: Patch coverage is 99.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 96.53%. Comparing base (fc25e04) to head (e43aca8).
Report is 53 commits behind head on main.

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     

[botberry]

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:

---

Operations over graphql-transport-ws now create the Context and perform validation on
the worker Task, thus not blocking the websocket from accepting messages.

Here's the tweet text:

🆕 Release (next) is out! Thanks to @kristjanvalur for the PR 👏

Get it here 👉 https://strawberry.rocks/release/(next)

[DoctorJohn]

Thanks for updating @kristjanvalur, still validating the changes 👍

[kristjanvalur]

PR is updated but I cannot see from the codecov that there is any missing coverage for my changes.

[kristjanvalur]

This PR needs to be completely refactored because of the upstream changes.
The "context" and "root value" are now created in a different place, long before the task is created. These really need to be evaluated in the context of the Task to not block the socket. I will attempt to re-factor this again.

[DoctorJohn]

This PR needs to be completely refactored because of the upstream changes. The "context" and "root value" are now created in a different place, long before the task is created. These really need to be evaluated in the context of the Task to not block the socket. I will attempt to re-factor this again.

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 get_context and get_root_value as lightweight methods. After recent refactors, both methods are only being called once per WebSocket connection.

Based on the assumption that these operations are lightweight, we were less concerned about get_context/get_root_value than the request validation mentioned in this PR's description. Feel free to split the request validation optimization in a separate PR so that we do not delay the merging of either part of this PR by pondering the other.

[kristjanvalur]

Sounds good. I'll leave the context/root as they are and rework this to just do the validation.

[kristjanvalur]

It appears that validation now happens in the task ,because the result_source is awaited inside the Task now. So presumably, validation now happens as part of that, in a separate Task. Since context is now created once for each connection, this leaves nothing left for this PR to do. All it could contribute is perhaps tests that validation does not block other messages for the connection. I'm inclined to simply close this.

[kristjanvalur]

Actually, my tests as written for the old PR do indicate a discrepancy in validation handling for queries vs subscriptions.
I'll create a new PR with just the tests, showing the problem, without proposing a solution.
see #3671. Please have a look at this and give me your opinion.

[kristjanvalur]

By the way, speaking of "context" being created once for each WS connection, I want to draw your attention to this bug: #1754

[bellini666]

My $.02 cents in here, leaving for @DoctorJohn to actually approve this once it is ready

[bellini666]

nit: since we enforce typing a lot here, assert <something> is a well known practice for type checkers, so I don't think the comment is required

Suggested change

	assert task is not None # for typecheckers
	assert task is not None

[bellini666]

Should this be wrapped in a with suppress(asyncio.CancelledError)?

[DoctorJohn]

It appears that validation now happens in the task ,because the result_source is awaited inside the Task now. So presumably, validation now happens as part of that, in a separate Task. Since context is now created once for each connection, this leaves nothing left for this PR to do. All it could contribute is perhaps tests that validation does not block other messages for the connection. I'm inclined to simply close this.

Thanks for verifying! It looks like this was changed when extension support for subscriptions was added. Let's close this PR then.

Actually, my tests as written for the old PR do indicate a discrepancy in validation handling for queries vs subscriptions.
I'll create a new PR with just the tests, showing the problem, without proposing a solution.
see #3671. Please have a look at this and give me your opinion.

Thank you so much for highlighting that. I'll check the specs and the PR introducing this discrepancy again. It looks like something slipped through the review and had no proper tests.

By the way, speaking of "context" being created once for each WS connection, I want to draw your attention to this bug: #1754

👍 Currently reading through it.