fix(incremental): abort async iteration when stream errors or stream is filtered #3815
Conversation
✅ Deploy Preview for compassionate-pike-271cb3 ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
|
Hi @yaacovCR, I'm @github-actions bot happy to help you with this PR 👋 Supported commandsPlease post this commands in separate comments and only one per comment:
|
yaacovCR
force-pushed
the
stream-test-failures
branch
from
b9cdcb4 to
c785f21
Compare
yaacovCR
changed the title
|
@robrichard @graphql/graphql-js-reviewers tests fixed by adding a context object scoped to each stream that can be used to check/set error status of the entire stream. |
yaacovCR
force-pushed
the
stream-test-failures
branch
2 times, most recently
from
507ba85 to
deb73ea
Compare
|
Woops, initial approach worked, but was not robust, i.e. didn't end up fixing similar problem with the new Publisher. The wrinkle is that errors outside of a stream that would lead to the entire stream being filtered should also abort async iteration. This can be managed by having a set of running streams handled by the publisher/dispatcher (currently simply stored on |
yaacovCR
changed the title
yaacovCR
force-pushed
the
stream-test-failures
branch
3 times, most recently
from
07b6036 to
1f1533f
Compare
Currently: When list item completion fails for a stream with a non-nullable list: 1. the entire list must be nulled within the given AsyncPayloadRecord 2. any other pending AsyncPayloadRecords must be filtered 3. async iteration powering the stream must be cancelled Currently, the third objective is accomplished by way of the second; during AsyncPayloadRecord filtering, if a stream record is filtered and has an associated asyncIterator, its return() method is called, which _should_ end the stream. This can go wrong in a few ways: A: The return() method may not exist; by specification, the return() method exists for the caller to notify the callee that the caller no longer intends to call next(), allowing for early cleanup. The method is optional, however, and so should not be relied on. B: The return method, even if it exists, may not be set up to block any next() calls while it operates. Async generators have next and return methods that always settle in call order, but async iterables do not.
yaacovCR
force-pushed
the
stream-test-failures
branch
from
1f1533f to
0d508d2
Compare
|
closing in favor of #3886 |
Currently:
When list item completion fails for a stream with a non-nullable list:
Currently, the third objective is accomplished by way of the second; during AsyncPayloadRecord filtering, if a stream record is filtered and has an associated asyncIterator, its return() method is called, which should end the stream.
This can go wrong in a few ways:
A: The return() method may not exist; by specification, the return() method exists for the caller to notify the callee that the caller no longer intends to call next(), allowing for early cleanup. The method is optional, however, and so should not be relied on.
B: The return method, even if it exists, may not be set up to block any next() calls while it operates. Async generators have next and return methods that always settle in call order, but async iterables do not.
This PR adds tests addressing these scenarios and fixes the test failures.