introduce new Publisher for incremental delivery #3786
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
new-pub
branch
2 times, most recently
from
1a7528a to
fe3dfbc
Compare
|
This PR depends on #3784 which extracts out the current publisher from the execution workflow without changing any tests. The final commit is the single commit unique to this PR that swaps out the old publisher for the new and demonstrates how the executor uses it differently and how batching of incremental payloads change.
Currently, the executor adds a The new Publisher introduces a new
As a result of the fact that an |
Corresponds to graphql/graphql-js#3786
Corresponds to graphql/graphql-js#3786
|
This PR corresponds to a proposed PR on the existing "incremental delivery" spec PR: It turns out that the existing spec PR have introduced this asynchronous chaining into the spec as well. The associated spec PR follows the new implementation in removing it. |
yaacovCR
force-pushed
the
new-pub
branch
2 times, most recently
from
4da741c to
61260af
Compare
Corresponds to graphql/graphql-js#3786
Depends on graphql#3784 The proposed new Publisher: 1. does not use the event loop to manage AsyncRecord dependencies 2. performs as much work as possible synchronously 2. uses separate sets to store pending vs ready AsyncRecords 3. does not use `Promise.race` -- No event loop for managing AsyncRecord dependencies The current Publisher wraps every AsyncRecord result in a promise that only resolves after the parent AsyncRecord resolves. If multiple items within a stream are released for publishing because their parent has just been published, the stream cannot be in its entirety until after all of these promises unwind. The new Publisher keeps track of dependencies manually. When an AsyncRecord is pushed by the publisher, all of its dependent AsyncRecords are synchronously pushed, repeating as necessary, without using the event loop. In general, the new publisher aims to perform as much work as possible synchronously. -- Separate sets for pending vs ready AsyncRecords The current publisher inspects all pending AsyncRecords whenever any of them resolves. All that are completed are added to the response. The new Publisher moves AsyncRecords from the pending set to the ready set as they are pushed, so that on each call to next, only the ready set must be iterated. As a side-effect of this change, the incremental array is ordered by which items are ready for delivery first, and not by the initial document. This seems like a worthwhile tradeoff, and is still adherent to the spec, as far as I can tell. -- No `Promise.race` The old Publisher uses `Promise.race` as the trigger to determine whether payloads are ready. The new Publisher uses a single triggering promise that is triggered whenever an AsyncRecord is pushed, and then reset. This may be beneficial as the implementation of `Promise.race` within V8 has a known memory leak for long-running promises. (see https://bugs.chromium.org/p/v8/issues/detail?id=9858). An alternative would be to utilize @brainkim 's memory-safe version detailed in that issue.
|
Failing tests for this PR after rebasing on main appear to have to do with bug identified in #3815. Apparently, the new Publisher is able to surface this issue with the current test suite after #3754 landed. #3815 will need to be solved separately in main, and presumably this PR can be rebased on top of that. |
|
this was reworked and merged as #3894 |
Depends on #3784
The proposed new Publisher:
Promise.raceNo event loop for managing AsyncRecord dependencies
The current Publisher wraps every AsyncRecord result in a promise that only resolves after the parent AsyncRecord resolves. If multiple items within a stream are released for publishing because their parent has just been published, the stream cannot be in its entirety until after all of these promises unwind.
The new Publisher keeps track of dependencies manually. When an AsyncRecord is pushed by the publisher, all of its dependent AsyncRecords are synchronously pushed, repeating as necessary, without using the event loop.
Separate sets for pending vs ready AsyncRecords
The current publisher inspects all pending AsyncRecords whenever any of them resolves. All that are completed are added to the response. The new Publisher moves AsyncRecords from the pending set to the ready set as they are pushed, so that on each call to next, only the ready set must be iterated.
As a side-effect of this change, the incremental array is ordered by which items are ready for delivery first, and not by the initial document.
This seems like a worthwhile tradeoff, and is still adherent to the spec, as far as I can tell.
No
Promise.raceThe old Publisher uses
Promise.raceas the trigger to determine whether payloads are ready. The new Publisher uses a single triggering promise that is triggered whenever an AsyncRecord is pushed, and then reset. This may be beneficial as the implementation ofPromise.racewithin V8 has a known memory leak for long-running promises. (see https://bugs.chromium.org/p/v8/issues/detail?id=9858). An alternative would be to utilize @brainkim 's memory-safe version detailed in that issue.