fix(race): concurrent next calls with defer/stream #2975
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yaacovCR
force-pushed
the
fix-conflicting-next
branch
3 times, most recently
from
f70ec66 to
c3f9508
Compare
yaacovCR
changed the title
yaacovCR
force-pushed
the
fix-conflicting-next
branch
from
c3f9508 to
9680b06
Compare
This was referenced Mar 19, 2021
yaacovCR
force-pushed
the
fix-conflicting-next
branch
from
3fd57a8 to
e272823
Compare
|
@robrichard @IvanGoncharov ready for review |
|
Although this change suggests that _subsequentPayloads should be a set rather than an array |
|
This PR fixes by modifying the general approach as little as possible, we still create Promise reactions to all the pending promises on each call to See #2848 (comment) where I link to https://github.com/repeaterjs/repeater/blob/219a0c8faf2c2768d234ecfe8dd21d455a4a98fe/packages/repeater/src/repeater.ts#L687 that uses a different approach to convert all pending promises to async iterables (which each yield one value) and then combining those iterables in settle order. If you would be more open to that type of approach, I can try to see if I can work to extract it (maybe with the help of @brainkim) |
|
@yaacovCR thanks again for your help on this. I'm not tied to the current approach. I think a generic well-tested jsutil that can combine and race promises and async iterables makes sense. |
|
@robrichard @IvanGoncharov I think the best path forward so as to not lose steam/forward momentum with any experimenters, would be to merge this bug fix that keeps the existing approach, and then as a separate PR, see if myself or @brainkim is able to work something else out in a refactor. |
IvanGoncharov
added
the
stream/defer
yaacovCR
added a commit
to ardatan/graphql-tools
that referenced
this pull request
Apr 21, 2021
tests failing, on the bright side, it means that our test coverage is meaningful :) at least some tests are failing because of a bug in current defer/stream implementation, see graphql/graphql-js#2975
yaacovCR
added a commit
to ardatan/graphql-tools
that referenced
this pull request
May 1, 2021
17 tasks
|
Would this be accepted if rebased? graphql tools stitching support uses patch package to apply this fix.... |
|
@yaacovCR I'm planning to spend some time refactoring the dispatcher to a generic jsutil that can be more easily tested and handle all the edge cases |
|
Sure but until then can we merge it so the bug is fixed |
yaacovCR
force-pushed
the
fix-conflicting-next
branch
from
ace3509 to
1eaed52
Compare
|
Meaning prior to next experimental release it would be nice if fixed somehow with this PR if not a refactor |
robrichard
added a commit
that referenced
this pull request
Feb 2, 2022
* fix(race): concurrent next calls * refactor test * use invariant * disable eslint error * fix
robrichard
added a commit
that referenced
this pull request
Feb 7, 2022
* fix(race): concurrent next calls * refactor test * use invariant * disable eslint error * fix
robrichard
added a commit
that referenced
this pull request
Feb 7, 2022
* fix(race): concurrent next calls * refactor test * use invariant * disable eslint error * fix
robrichard
added a commit
that referenced
this pull request
Feb 23, 2022
* fix(race): concurrent next calls * refactor test * use invariant * disable eslint error * fix
robrichard
added a commit
that referenced
this pull request
Feb 23, 2022
* fix(race): concurrent next calls * refactor test * use invariant * disable eslint error * fix
robrichard
added a commit
that referenced
this pull request
Feb 23, 2022
* fix(race): concurrent next calls * refactor test * use invariant * disable eslint error * fix
robrichard
added a commit
that referenced
this pull request
Mar 7, 2022
* fix(race): concurrent next calls * refactor test * use invariant * disable eslint error * fix
robrichard
added a commit
that referenced
this pull request
Mar 7, 2022
* fix(race): concurrent next calls * refactor test * use invariant * disable eslint error * fix
robrichard
added a commit
that referenced
this pull request
Apr 4, 2022
* fix(race): concurrent next calls * refactor test * use invariant * disable eslint error * fix
robrichard
added a commit
that referenced
this pull request
Apr 13, 2022
* fix(race): concurrent next calls * refactor test * use invariant * disable eslint error * fix
robrichard
added a commit
that referenced
this pull request
Apr 20, 2022
* fix(race): concurrent next calls * refactor test * use invariant * disable eslint error * fix
robrichard
added a commit
that referenced
this pull request
Apr 20, 2022
* fix(race): concurrent next calls * refactor test * use invariant * disable eslint error * fix
robrichard
added a commit
that referenced
this pull request
Apr 27, 2022
* fix(race): concurrent next calls * refactor test * use invariant * disable eslint error * fix
robrichard
added a commit
that referenced
this pull request
May 5, 2022
* fix(race): concurrent next calls * refactor test * use invariant * disable eslint error * fix
robrichard
added a commit
that referenced
this pull request
May 5, 2022
* fix(race): concurrent next calls * refactor test * use invariant * disable eslint error * fix
robrichard
added a commit
that referenced
this pull request
May 13, 2022
* fix(race): concurrent next calls * refactor test * use invariant * disable eslint error * fix
robrichard
added a commit
that referenced
this pull request
May 23, 2022
* fix(race): concurrent next calls * refactor test * use invariant * disable eslint error * fix
robrichard
added a commit
that referenced
this pull request
May 26, 2022
* fix(race): concurrent next calls * refactor test * use invariant * disable eslint error * fix
robrichard
added a commit
that referenced
this pull request
May 26, 2022
* fix(race): concurrent next calls * refactor test * use invariant * disable eslint error * fix
robrichard
added a commit
that referenced
this pull request
Jun 13, 2022
* fix(race): concurrent next calls * refactor test * use invariant * disable eslint error * fix
robrichard
added a commit
that referenced
this pull request
Jun 21, 2022
* fix(race): concurrent next calls * refactor test * use invariant * disable eslint error * fix
robrichard
added a commit
that referenced
this pull request
Jun 22, 2022
* fix(race): concurrent next calls * refactor test * use invariant * disable eslint error * fix
robrichard
added a commit
that referenced
this pull request
Jun 23, 2022
* fix(race): concurrent next calls * refactor test * use invariant * disable eslint error * fix
robrichard
added a commit
that referenced
this pull request
Aug 3, 2022
* fix(race): concurrent next calls * refactor test * use invariant * disable eslint error * fix
robrichard
added a commit
that referenced
this pull request
Aug 3, 2022
* fix(race): concurrent next calls * refactor test * use invariant * disable eslint error * fix
robrichard
added a commit
that referenced
this pull request
Aug 15, 2022
* fix(race): concurrent next calls * refactor test * use invariant * disable eslint error * fix
robrichard
added a commit
that referenced
this pull request
Aug 19, 2022
* fix(race): concurrent next calls * refactor test * use invariant * disable eslint error * fix
robrichard
added a commit
that referenced
this pull request
Aug 19, 2022
* fix(race): concurrent next calls * refactor test * use invariant * disable eslint error * fix
robrichard
added a commit
that referenced
this pull request
Aug 23, 2022
* fix(race): concurrent next calls * refactor test * use invariant * disable eslint error * fix
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This seems to work with me for my project.
not quite sure what to do about isDone which seems to be used just when return is called (parenthetically, should that be renamed to isReturned for clarity/distinction between
isDoneanddoneattributes?)namely, should the isDone check go up top prior to checking for the next resolved promise, within the next resolved promise check, or maybe both.
in general, not sure how we are really handling secure stopping issues, seems like -- for example -- there could be condition in which we await a promise which never resolves, but that is ok, because return has been called?
but still quite the novice with all of this async stuff
---From my opinionated perspective, I realize that graphql-js is a reference lib, and has a zero-dependency footprint, but seems like handling all these async issues (in JS in general and within
graphql-js) would be easier with a Repeater-like construct (see https://repeater.js.org/, @brainkim) that could be even theoretically inlined in downsized version intojs-utils.