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

Eagerly start subscription heartbeats #7871

Merged
merged 4 commits into from
Apr 18, 2024
Merged

Conversation

tninesling
Copy link
Contributor

Overview

Eagerly start subscription heartbeats in case the subscription has long-running setup steps before returning its async generator. When a subscription is configured, subscribe can return either an AsyncGenerator or a Promise<AsyncGenerator>. If an AsyncGenerator is returned, the previous code works fine. We get the generator right away and set up the heartbeat to keep things alive, even if the generator has long-running setup code before returning values.

On the other hand, if the Promise<AsyncGenerator> takes longer than the hearbeat interval to resolve, the router will terminate the subscription. When the subscription actually starts generating data, the callback returns 404, as it has already been cleaned up. We want the system to gracefully wait for the setup code to complete, since we will do so if the setup code is placed inside the iterator itself.

Example

In this example, subscribe returns a Promise<AsyncGenerator>, but the Promise resolves quickly. This results in the heartbeat being started prior to the long-running setup. The heartbeat keeps the subscription alive until the setup is completed, then it can start sending data to the client.

return {
    subscribe: async () => {
        return {
            async *[Symbol.asyncIterator]() {
                // The heartbeat is already started, so this can wait as long as it wants
                await setupWithLongDelay();
                while (running) {
                    yield data;
                }
            },
        };
    },
}

On the other hand, waiting for the setup before resolving the Promise leads to complications.

return {
    subscribe: async () => {
        // The router kills this request because it doesn't start the heartbeat in time
        await setupWithLongDelay(); 
        return {
            async *[Symbol.asyncIterator]() {
                while (running) {
                    yield data;
                }
            },
        };
    },
}

Solution

This change eagerly starts the heartbeat interval prior to awaiting the returned value from subscribe. This allows long-running setup code to run concurrently with the heartbeats. Previously the second example would result in a SUBSCRIPTION_HEARTBEAT_ERROR and subscription termination. Now, the router receives heartbeats so it correctly waits for the setup and data in both cases.

…ng-running setup steps before returning its async generator
Copy link

netlify bot commented Apr 17, 2024

Deploy Preview for apollo-server-docs ready!

Name Link
🔨 Latest commit 63b5ce1
🔍 Latest deploy log https://app.netlify.com/sites/apollo-server-docs/deploys/66212a20476622000753001e
😎 Deploy Preview https://deploy-preview-7871--apollo-server-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codesandbox-ci bot commented Apr 17, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@tninesling tninesling marked this pull request as ready for review April 17, 2024 21:08
Copy link
Member

@trevor-scheer trevor-scheer left a comment

Choose a reason for hiding this comment

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

Looks great!

packages/server/src/plugin/subscriptionCallback/index.ts Outdated Show resolved Hide resolved
@tninesling tninesling merged commit 18a3827 into main Apr 18, 2024
22 checks passed
@tninesling tninesling deleted the tninesling/eager-heartbeats branch April 18, 2024 14:47
@github-actions github-actions bot mentioned this pull request Apr 18, 2024
tninesling pushed a commit that referenced this pull request Apr 18, 2024
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## @apollo/server-integration-testsuite@4.10.4

### Patch Changes

- Updated dependencies
\[[`18a3827`](18a3827)]:
    -   @apollo/server@4.10.4

## @apollo/server@4.10.4

### Patch Changes

- [#7871](#7871)
[`18a3827`](18a3827)
Thanks [@tninesling](https://github.com/tninesling)! - Subscription
heartbeats are initialized prior to awaiting subscribe(). This allows
long-running setup to happen in the returned Promise without the
subscription being terminated prior to resolution.

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants