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

upgrade executor to non-duplicating incremental delivery format #6243

Closed
wants to merge 2 commits into from

Conversation

yaacovCR
Copy link
Collaborator

@yaacovCR yaacovCR commented Jun 5, 2024

Description

GraphQL Incremental Delivery is moving to a new response format without duplication.

This PR updates the executor within graphql-tools to follow the new format, with my best effort to integrate the changes made within the graphql-tools executor, largely around abortion.

Notes:

  • This version of incremental delivery has a new response format. The loader package has been updates to work with the new response format (mergeIncrementalResults()), and Yoga tests that rely on it pass, but there should probably eventually be new unit tests new unit tests have been added within the existing mergeIncrementalResults() tests. The function still work with the old response format.
  • The new response format itself is a BREAKING CHANGE, clients expecting an executor that emits the old format will no longer have path references where they expect, instead they will get ids and be required to lookup the path based on the information from pending.
  • This version is a straight-copy from graphql-js where a decision has been made to disable early execution by default. Some believe that despite theoretical causes for concern, a better default would be to enable early execution. This version enables early execution by default, with a flag to disable, differing from graphql-js where early execution will probably be disabled by default, at least initially.
  • The abortion logic for stream has been modified to be somewhat similar and to avoid Promise.race, although this may mean that abortion might get stuck while waiting for the next payload if the underlying iterator does not properly handle a return(). The rationale for avoiding Promise.race is the memory leak that may occur with long-running promises. This change should probably be discussed further before merging. See the comment below for how this is accomplished.
  • My work on Incremental Delivery during 2023 was sponsored by the Guild, presented at GraphQL Conf 2023. The main blocker to release has been the spec changes, where the best prose has been elusive.
  • It would be amazing for the community to get the new format more widely tested. That's what this PR is about.
  • Hopefully, this will get us closer to incremental delivery being released. Until, then, we have Fitzgerald: "So we beat on, boats against the current, borne back ceaselessly into the past."

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as
    expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can
reproduce. Please also list any relevant details for your test configuration

  • graphql-js tests have been transferred.
  • Existing graphql-tools tests have been updated.

Test Environment:

  • OS:
  • @graphql-tools/...: latest
  • NodeJS: 22

Checklist:

  • I have followed the
    CONTRIBUTING doc and the
    style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests and linter rules pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Copy link

changeset-bot bot commented Jun 5, 2024

🦋 Changeset detected

Latest commit: 2c6fb0f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
@graphql-tools/utils Minor
@graphql-tools/executor Major
@graphql-tools/delegate Patch
@graphql-tools/stitch Patch
federation-benchmark Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

github-actions bot commented Jun 5, 2024

✅ Benchmark Results

     ✓ no_errors
     ✓ expected_result

     checks.........................: 100.00% ✓ 320       ✗ 0  
     data_received..................: 37 MB   3.7 MB/s
     data_sent......................: 137 kB  14 kB/s
     http_req_blocked...............: avg=4.39µs   min=2.08µs   med=3.15µs   max=179.53µs p(90)=4.09µs   p(95)=4.38µs  
     http_req_connecting............: avg=755ns    min=0s       med=0s       max=120.86µs p(90)=0s       p(95)=0s      
     http_req_duration..............: avg=58.21ms  min=48.57ms  med=54.62ms  max=160.25ms p(90)=63.26ms  p(95)=91.27ms 
       { expected_response:true }...: avg=58.21ms  min=48.57ms  med=54.62ms  max=160.25ms p(90)=63.26ms  p(95)=91.27ms 
     http_req_failed................: 0.00%   ✓ 0         ✗ 160
     http_req_receiving.............: avg=137.52µs min=107.29µs med=131.25µs max=310.52µs p(90)=159.91µs p(95)=168.81µs
     http_req_sending...............: avg=26.41µs  min=17.28µs  med=25.32µs  max=122.64µs p(90)=33.3µs   p(95)=36.9µs  
     http_req_tls_handshaking.......: avg=0s       min=0s       med=0s       max=0s       p(90)=0s       p(95)=0s      
     http_req_waiting...............: avg=58.05ms  min=48.42ms  med=54.47ms  max=159.91ms p(90)=63.07ms  p(95)=91.08ms 
     http_reqs......................: 160     15.957895/s
     iteration_duration.............: avg=62.64ms  min=52.6ms   med=59.19ms  max=164.49ms p(90)=69.17ms  p(95)=95.26ms 
     iterations.....................: 160     15.957895/s
     vus............................: 1       min=1       max=1
     vus_max........................: 1       min=1       max=1

Copy link
Contributor

github-actions bot commented Jun 5, 2024

🚀 Snapshot Release (alpha)

The latest changes of this PR are available as alpha on npm (based on the declared changesets):

Package Version Info
@graphql-tools/delegate 10.0.19-alpha-20240813170106-2c6fb0fc8fa322bcea8c10c35972ba621a04c4a4 npm ↗︎ unpkg ↗︎
@graphql-tools/executor 2.0.0-alpha-20240813170106-2c6fb0fc8fa322bcea8c10c35972ba621a04c4a4 npm ↗︎ unpkg ↗︎
@graphql-tools/stitch 9.2.11-alpha-20240813170106-2c6fb0fc8fa322bcea8c10c35972ba621a04c4a4 npm ↗︎ unpkg ↗︎
@graphql-tools/utils 10.6.0-alpha-20240813170106-2c6fb0fc8fa322bcea8c10c35972ba621a04c4a4 npm ↗︎ unpkg ↗︎

Copy link
Contributor

github-actions bot commented Jun 5, 2024

💻 Website Preview

The latest changes are available as preview in: https://4fbf8b14.graphql-tools.pages.dev

@yaacovCR
Copy link
Collaborator Author

yaacovCR commented Jun 5, 2024

Some additional issues:

  • The existing changeset in this PR was added automatically, I guess. The appropriate changeset would be BREAKING for the executor, and I guess for the loader? And a MINOR for utils as processing the new format is I guess a new feature. (EDIT: this has been done!)
  • The executor package gets its own new version of collectFields. Perhaps that could be integrated into the existing collectFields that is in utils, but presumably after incremental delivery is finalized.

@EmrysMyrddin
Copy link
Collaborator

For changeset, you can run yarn changeset to add new changesets with the desired level and message :-)

@yaacovCR
Copy link
Collaborator Author

yaacovCR commented Jun 5, 2024

We can use the approach from graphql/graphql-js#4043 which improves code readability and includes a significant performance benefit. At graphql-js, we have not yet adopted this approach, because we will be following the spec more closely, at least during the initial adoption period.

@yaacovCR
Copy link
Collaborator Author

yaacovCR commented Jun 5, 2024

I forgot to mention that another BREAKING CHANGE is that incremental delivery no longer is supported within subscriptions.

In theory, we can remote flattenAsyncIterable() but the error handling in subscriptions becomes a bit different than in current tests if it is simply removed, which I have not gotten to the bottom of which I have now done.

@yaacovCR
Copy link
Collaborator Author

yaacovCR commented Jun 6, 2024

I've updated this PR to re-enable early execution by default.

@yaacovCR
Copy link
Collaborator Author

yaacovCR commented Jun 6, 2024

Is it possible that we would want this executor to emit the old duplicated format by default and enable the new format under a flag?

@yaacovCR
Copy link
Collaborator Author

yaacovCR commented Jun 6, 2024

I believe I've fixed the issue with aborting from incremental delivery despite pending long-running promises, still avoiding the use of Promise.race().

The key was to realize that the IncrementalPublisher awaits the next result from IncrementalGraph which uses a hand-crafted AsyncGenerator rather than a "true" AsyncGenerator. A hand-crafted AsyncGenerator can -- and in this case does -- cause the Promises returned by .next() methods to return early if .return() is called in parallel. This allows a way to abort the await early without using Promise.race().

I've added a test demonstrating this.

@yaacovCR
Copy link
Collaborator Author

yaacovCR commented Jun 6, 2024

I've added the new unit tests for mergeIncrementalResults.

@yaacovCR
Copy link
Collaborator Author

yaacovCR commented Jun 9, 2024

Is it possible that we would want this executor to emit the old duplicated format by default and enable the new format under a flag?

Thinking about this more, this is definitely possible in terms of functionality, but there would be a breaking change in terms of the types -- or we would have to introduce a new set of types for the new format with maybe a prefix or suffix.

I am not unsure how to proceed, definitely easier just to make it a BREAKING CHANGE, so at this point I will leave this PR as is => From my perspective, this is now ready for review! 🚀

@n1ru4l
Copy link
Collaborator

n1ru4l commented Jul 1, 2024

@yaacovCR Now catching up with your work and rewatching your video from GraphQL Conf was very helpful! 🙏

I am fine with shipping this as a breaking change. But, we should have a middleware/wrapper function that takes the execute function and emits the old format. That way we can at least provide people with a migration path within Yoga that can not upgrade all their client's code immediately!

@yaacovCR
Copy link
Collaborator Author

yaacovCR commented Jul 1, 2024

Sounds good, I can work on that.

@yaacovCR yaacovCR force-pushed the new-incremental branch 3 times, most recently from 8170044 to d921d60 Compare July 2, 2024 18:20
@yaacovCR
Copy link
Collaborator Author

yaacovCR commented Jul 2, 2024

I added an argument to toggle whether incremental is disabled for subscriptions so that should cover that breaking change. I'm trying to decide whether to add an argument for deduplication or whether to add a wrapper, I'm thinking of going with the former.

As an aside, I think tests are broken separate from this PR.

@yaacovCR
Copy link
Collaborator Author

yaacovCR commented Jul 9, 2024

@ardatan @n1ru4l in the above checks — can I check to see the performance change with this PR? Do you know which spots to best look at?

@yaacovCR
Copy link
Collaborator Author

Rebased! Enjoy!

@ardatan @n1ru4l in the above checks — can I check to see the performance change with this PR? Do you know which spots to best look at?

@n1ru4l
Copy link
Collaborator

n1ru4l commented Aug 3, 2024

@yaacovCR can you please rebase? I want to have an up2date alpha release fo testing on GraphQL Yoga!

@yaacovCR yaacovCR force-pushed the new-incremental branch 2 times, most recently from f5533b6 to 7dfd592 Compare August 4, 2024 11:54
@yaacovCR
Copy link
Collaborator Author

yaacovCR commented Aug 4, 2024

@yaacovCR can you please rebase? I want to have an up2date alpha release fo testing on GraphQL Yoga!

Done.

Comment on lines 21 to 24
deduplicateDefers: false,
sendIncrementalErrorsAsNull: true,
sendPathAndLabelOnIncremental: true,
errorWithIncrementalSubscription: false,
Copy link
Collaborator

@n1ru4l n1ru4l Aug 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When providing these parameters, I still receive {"data":{},"pending":[{"id":"0","path":[]}],"hasNext":true} instead of {"data":{},"hasNext":true} and also id payloads.

      Array [
        Object {
          "data": Object {},
          "hasNext": true,
    +     "pending": Array [
    +       Object {
    +         "id": "0",
    +         "path": Array [],
    +       },
    +     ],
        },
    +   Object {
    +     "completed": Array [
            Object {
    +         "id": "0",
    +       },
    +     ],
          "hasNext": false,
          "incremental": Array [
            Object {
              "data": Object {
                "a": "a",
              },
    +         "id": "0",
              "path": Array [],
            },
          ],
        },
      ]

Seems like these should also be omitted to be fully backwards-compatible? (See failing tests in dotansimha/graphql-yoga#3402)


Regarding all the granular options - do you think we need to allow such granularly for the user? I think from our perspective they would only care about "old" spec and "new" spec, not on using only parts of the new spec? 🤔

So maybe instead we should use a version number? Do you see benefit in setting some of the flags and omitting others from an end-user and feedback-user perspective?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t think extra graphql fields are technically backwards incompatible as anything custom goes into extensions.

But I’m certainly open to an additional option to suppress the pending/completed which would require setting sendPathAndLabelOnIncremental to true.

We could also add a version option that implies a different combination of defaults so that the granular options could be simplified

I think I am also noticing that the errors are now sent after data, which is not preferred according to the specification it should also be fixed

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t think extra graphql fields are technically backward incompatible as anything custom goes into extensions.

It is still a different output than expected, and strict clients might react funky... I am unaware of any clients that do so right now, but I can already imagine this question coming up from a user. Since it seems straightforward to implement, let's add it.


We could also add a version option that implies a different combination of defaults so that the granular options could be simplified

Is there some reference version name e.g. date or similar used within the working group context to differentiate the different drafts?


What do you think about grouping the defer/stream-related options within an object? For the user-facing API, they would be more organized and separated from the other options.

execute({
  ...otherArgs,
  incrementalDelivery: {
    deduplicateDefers: false,
    sendIncrementalErrorsAsNull: true,
    sendPathAndLabelOnIncremental: true,
    errorWithIncrementalSubscription: false,
  }
})

In addition or instead, if the granular configuration of deduplicateDefers, sendIncrementalErrorsAsNull, sendPathAndLabelOnIncremental, errorWithIncrementalSubscription (and also an option for dropping the additional new properties - we can do something like the following:

execute({
  ...otherArgs,
  incrementalDelivery:  "2024-08-working-draft"
})

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reworked it as follows as described in the changeset:

Library users can explicitly opt in to the older format by call execute with the following option:

const result = await execute({
  ...,
  incrementalPreset: 'v17.0.0-alpha.2',
});

The default value for incrementalPreset when omitted is 'v17.0.0-alpha.3', which enables the new behaviors described above. The new behaviors can also be disabled granularly as follows:

const result = await execute({
  ...,
  deferWithoutDuplication: false,
  useIncrementalNotifications: false,
  errorOnSubscriptionWithIncrementalDelivery: false,
});

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is some renaming and some consolidation => the use of ids is intimately connected to the use of pending/completed for notifications in whose absence it makes sense to send incremental errors as null entries. This seems more concise.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am also totally fine with any regrouping/renaming tweaks you might want to make. in terms of levels of granularity, I think it might theoretically make sense to preserve ability to toggle notifications separately than deduplication, but I am not aware of any actual use case yet for that. I believe the long-term plan is definitely to revisit incremental delivery with subscriptions so that has a definite argument for retaining as a separate parameter although the truth is, it could all be revisited later. I don’t have much strong feeling about any of this portion of the api….

includes new options to allow for prior branching format
@yaacovCR
Copy link
Collaborator Author

yaacovCR commented Oct 9, 2024

Closing this for now. I think it overall might be easier to upstream the changes from this executor into graph JS rather than porting over incremental delivery, fragment arguments, and the other recent changes at graphql js around validation.

Let me know if there is any renewed interest on your end!

@yaacovCR yaacovCR closed this Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants