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: subscriptions not closing after done #1793

Merged
merged 14 commits into from Feb 26, 2024
Merged

Conversation

nedsalk
Copy link
Contributor

@nedsalk nedsalk commented Feb 23, 2024

This fix was part of #1597 which didn't get merged. In essence, the piping done in

const subscriptionStreamReader = response
    .body!.pipeThrough(new FuelSubscriptionStream())
    .getReader();

was ignoring the value {value: undefined, done: true} which indicates a stream is done. This resulted in subscriptions stalling when they aren't cancelled externally with a break in a for-await-of loop even though their underlying connection got closed properly. I opted for not using piping for transforming streams but rather inlining the byte parsing in the next method of the iterator.

You can see more around the investigation of this problem in #1615 (comment).

Besides this, the PR also had a test that verified our code correctly handles keep-alive messages for long-running subscriptions which I added here as well.

@nedsalk nedsalk marked this pull request as draft February 23, 2024 09:46
@nedsalk nedsalk marked this pull request as ready for review February 23, 2024 09:52
@Torres-ssf Torres-ssf added the bug label Feb 23, 2024
arboleya

This comment was marked as resolved.

Co-authored-by: Anderson Arboleya <anderson@arboleya.me>
Copy link
Member

@arboleya arboleya left a comment

Choose a reason for hiding this comment

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

I like the new direction. 🫡

@nedsalk nedsalk linked an issue Feb 26, 2024 that may be closed by this pull request
@nedsalk nedsalk enabled auto-merge (squash) February 26, 2024 15:04
Copy link
Contributor

Coverage Report:

Lines Branches Functions Statements
79.43%(+0.01%) 70.17%(+0%) 77.91%(+0.01%) 79.51%(+0.01%)
Changed Files:
Ok File (✨=New File) Lines Branches Functions Statements
🔴 packages/account/src/providers/fuel-graphql-subscriber.ts 91.3%
(+1.3%)
62.5%
(+0%)
100%
(+0%)
91.3%
(+1.3%)

@nedsalk nedsalk merged commit 96a735a into master Feb 26, 2024
13 checks passed
@nedsalk nedsalk deleted the ns/fix/subscriptions-stalling branch February 26, 2024 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Subscriptions not closing after done
4 participants