Use our own iterator to iterate and transform subscription results

[kristjanvalur]

Provide a generator based iterator to iterate over and transform the subscription results from graphql core.

Description

In graphql-python/graphql-core#197 I descibe how the iterator provided by graphql core is
problematic. Its aclose() semantics are unexpected and iteration happens on a subtask. A much simpler approach is possible which is both more performant (no subtasks), more easily debuggable (call stack isn't broken on __anext__() and where await acancel() actually only returns when the iterator has been fully cancelled.

A concrete problem with the current iterator is that during the handling of a GeneratorExit exception, e.g. in a finally: clause entered because the generator is closed, it is likely that one receives a CancelledError if one tries to do any async operations. This makes it hard to write test which test the finalization of subscriptions, and will make it impossible to do proper cleanup of async resources in case of subscription cancellation. This does not happen using regular async-generators.

We inspect the iterator we get from graphql-core. If it appears to be the old type with its internal _close_event, we replace it with our own implementation.

This is intended as an interim solution until graphql-core provides a better iterator which more closely conforms to the semantics one is to expect from Python's AsyncGenerators.

The change helps avoid the strange "RuntimeErrors()" we were seeing when calling aclose() despite doing so in a legal
way from the same task which was iterating, and in a sense is a bugfix.

It should also be noted that an iterator implemented this way, with an Async Generator, does not require an explicit call to "aclose()" since it will be scheduled when it goes out of scope. But expliticly doing so may be prudent if one wants the cleanup to happen promptly.

Upstream I have submitted both In graphql-python/graphql-core#197 and graphql-python/graphql-core#199. It is unclear if and when these will ever be accepted. Regardless of that, this PR also provides a framework for adding support for extensions to subscriptions, since they would need a custom iterator anyway.

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

Merging #2724 (7616426) into main (4519c39) will decrease coverage by 0.02%.
The diff coverage is 94.73%.

❗ Current head 7616426 differs from pull request most recent head 8658699. Consider uploading reports for the commit 8658699 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2724      +/-   ##
==========================================
- Coverage   96.45%   96.44%   -0.02%     
==========================================
  Files         198      198              
  Lines        8244     8190      -54     
  Branches     1500     1482      -18     
==========================================
- Hits         7952     7899      -53     
+ Misses        184      183       -1     
  Partials      108      108              

[botberry]

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:

---

We use a simpler and more performant iterator to iterate over subscription results.

Here's the tweet text:

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

Get it here 👉 https://github.com/strawberry-graphql/strawberry/releases/tag/(next)

[kristjanvalur]

Can't find where the coverage tests are failing... Are these tests actually correct?

[patrick91]

@kristjanvalur I don't this so, not sure if it is a misconfiguration or just coverage being bad

[jthorniley]

I'd like to vote for this but also offer a suggestion / request - can the translation be moved further up the chain into the schema.subscribe method itself? The reason I ask is that for unit tests I directly call schema.subscribe without the transport handler on top, and I need to unit test the close behaviour.

In particular, with the unit tests, the fact that schema.subscribe returns an AsyncIterator instead of an AsyncGenerator is a minor annoyance as it means the type checker doesn't think there should be an aclose on the result, when there always is, so would it make sense to me to change the return type.

To clarify what I'm suggesting I've made a commit on top of this branch, though its just a suggestion (I've tested this works in my context but I'm not sure if it impacts the strawberry test suite/mypy): mixcloud@2fb568a

[kristjanvalur]

Yes, I see. This is not a bad idea, and it will help to simplify some other things as well. It probably ties in with some other discussions, such as this here: #2701

(subscriptions returns an iterator over graphql-core results, whereas other schema operations return straqberry's own result types.)

I guess that a very valid first step is that graphql's schema returns its own AsyncGenerator (or a result in case of error)), until we decide what kind of tranformation of the results themselves is necessary (I think it has mostly to do with extensions which aren't supported at the moment for subscriptions).

Allow me to modify this PR accordingly.

[kristjanvalur]

@jthorniley something like this?

The current iterator truly is a can of worms. Cancelling it can result in unpredictable behaviour. For example, during processing of a GeneratorExit in the finalizer, a CancelledError may also arrive. This makes any reasonable logic operations in the finalizer hard to implement.

the error currently visible in the the unit tests is actually due to this. We are trying to measure the responsiveness of code which isn't well predictable. I already tried making this unit test work without timing, but was foiled by the above, CancelledErrors coming out of nowhere.

[jthorniley]

Yep, in terms of the processing being done inside subscribe, this is in line with what I was thinking - thanks!

[kristjanvalur]

After fixing the subscription iterator, it is finally possible to get deterministic behaviour for subscription finalizers. Previously they could get bot GeneratorExit and a CancelledError. it made it impossible to do any async logic in there, like wait for condition variables etc. I have fixed the test verifying that subscription cancellation does not block the connection, by using synchronization primitives rather than timeout measruements.

[kristjanvalur]

Yep, in terms of the processing being done inside subscribe, this is in line with what I was thinking - thanks!

This can also now be expanded upon to add extension processing to subscriptions.

[kristjanvalur]

graphql-core has this functionality in https://github.com/graphql-python/graphql-core/releases/tag/v3.3.0a3
Since upcoming release also changes the signature of "subscribe()" to be hybrid-async, I think I think I should probably
close this PR in favour of #2784

[DoctorJohn]

Closing this one because the upstream patch landed and extension support for subscriptions has been added in another PR. Thanks so much for this PR and keeping it updated a few times.