Skip to content
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(stream): continue on iterator error values #2980

Conversation

yaacovCR
Copy link
Contributor

if the iterator errors when attempting to get the next value, we can assume subsequent calls to next will also error, and abort, but if we successfully get the next value, but it ends up triggering an error, we can continue, optimistic that the next value will not do so.

@yaacovCR
Copy link
Contributor Author

@robrichard @IvanGoncharov this is more of a question than a PR -- what is the desired behavior?

@robrichard
Copy link
Contributor

I don't think it's expected to receive more values after an async iterable throws an error. How is this handled with subscriptions?

@yaacovCR
Copy link
Contributor Author

That’s just it, this code is not about the iterator returning an error, that’s further down. This code section is an error encountered in completion of a list item. With iterables, the next item is allowed to be processed, shouldn’t it be the same with async iterables?

@yaacovCR
Copy link
Contributor Author

Added tests to explain

@robrichard
Copy link
Contributor

@yaacovCR should this target the asynciteratable branch (#2757)? It doesn't seem to be bug in defer/stream, but rather the related feature of allowing async iterable return values from resolvers.

@yaacovCR
Copy link
Contributor Author

yaacovCR commented Apr 6, 2021

Yes! Will rework when I have a moment to target that branch

@yaacovCR yaacovCR changed the base branch from defer-stream to asynciteratable April 11, 2021 19:12
@yaacovCR yaacovCR changed the base branch from asynciteratable to defer-stream April 11, 2021 19:29
@yaacovCR yaacovCR force-pushed the continue-on-async-iterator-error-value branch from 8ee3876 to cf0f829 Compare April 11, 2021 19:45
@yaacovCR yaacovCR changed the base branch from defer-stream to asynciteratable April 11, 2021 19:46
@yaacovCR yaacovCR force-pushed the continue-on-async-iterator-error-value branch 3 times, most recently from aec6fd9 to ac0de32 Compare April 11, 2021 19:58
if the iterator errors when attempting to get the next value, we can assume subsequent calls to next will also error, and abort, but if we successfully get the next value, but it ends up triggering an error, we can continue, optimistic that the next value will not do so.
@yaacovCR yaacovCR force-pushed the continue-on-async-iterator-error-value branch from ac0de32 to 4b1547d Compare April 11, 2021 20:05
@yaacovCR
Copy link
Contributor Author

@robrichard @IvanGoncharov

ready for review

yaacovCR added a commit to ardatan/graphql-tools that referenced this pull request May 1, 2021
@robrichard robrichard force-pushed the asynciteratable branch 2 times, most recently from f1ddb83 to 10f12a1 Compare June 2, 2021 00:45
@yaacovCR
Copy link
Contributor Author

yaacovCR commented Jun 2, 2021

Bump, would this be accepted if rebased?

@IvanGoncharov IvanGoncharov added the stream/defer Issues/PRs related to experimental steam/defer support label Jun 2, 2021
@robrichard
Copy link
Contributor

@yaacovCR I pulled this in while rebasing. Thanks!

@robrichard robrichard closed this Jun 2, 2021
@yaacovCR yaacovCR deleted the continue-on-async-iterator-error-value branch June 2, 2021 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stream/defer Issues/PRs related to experimental steam/defer support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants