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 client delay when subscription is terminated #7849

Conversation

TylerBloom
Copy link
Contributor

This PR addresses #7842 .

When configured with an HTTP callback, a terminated subscription does not fully clean up its resources immediately. Namely, the async generator that the subscription object uses is not closed (i.e. return() is not called). This means anything that is await-ing that promise will be waiting until the next message comes through the subscription.

The desired behavior is that calling terminateSubscription will also end the async generator.

To do this, the async generator is stored as a field in the subscription object, and its return() method is called in the terminateSubscription method.

@apollo-cla
Copy link

@TylerBloom: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

Copy link

netlify bot commented Mar 12, 2024

👷 Deploy request for apollo-server-docs pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit c7e514c

@TylerBloom TylerBloom force-pushed the fix-client-delay-on-subscription-close branch 2 times, most recently from d872ebc to 8292730 Compare March 12, 2024 15:20
Copy link

codesandbox-ci bot commented Mar 12, 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.

@TylerBloom TylerBloom force-pushed the fix-client-delay-on-subscription-close branch 6 times, most recently from 39c85a4 to 7890c28 Compare March 12, 2024 20:19
@TylerBloom TylerBloom force-pushed the fix-client-delay-on-subscription-close branch from 7890c28 to c7e514c Compare March 12, 2024 20:24
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.

LGTM!

@trevor-scheer trevor-scheer force-pushed the fix-client-delay-on-subscription-close branch from 860caf4 to c7e514c Compare March 12, 2024 21:52
@TylerBloom TylerBloom merged commit 29c391c into apollographql:main Mar 15, 2024
16 checks passed
@TylerBloom TylerBloom deleted the fix-client-delay-on-subscription-close branch March 15, 2024 18:07
@github-actions github-actions bot mentioned this pull request Mar 19, 2024
trevor-scheer pushed a commit that referenced this pull request Mar 22, 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/[email protected]

### Patch Changes

- Updated dependencies
\[[`c7e514c`](c7e514c)]:
    -   @apollo/[email protected]

## @apollo/[email protected]

### Patch Changes

- [#7849](#7849)
[`c7e514c`](c7e514c)
Thanks [@TylerBloom](https://github.com/TylerBloom)! - In the
subscription callback server plugin, terminating a subscription now
immediately closes the internal async generator. This avoids that
generator existing after termination and until the next message is
received.

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 Apr 17, 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.

3 participants