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

Subscription stream doesn't close when connection closes #1628

Closed
nedsalk opened this issue Jan 10, 2024 · 6 comments
Closed

Subscription stream doesn't close when connection closes #1628

nedsalk opened this issue Jan 10, 2024 · 6 comments
Assignees
Labels
bug Issue is a bug

Comments

@nedsalk
Copy link
Contributor

nedsalk commented Jan 10, 2024

For details on what led to this issue, you can refer to this comment.

When subscribing to the fuel node via submitAndAwait or statusChange, the node will close the connection after it returns a non-SubmittedStatus transaction status, as is explained here. This should lead to the reader that's reading the stream sent from the fuel node to close as well, which would be signified by it returning {value: undefined, done: true}. This isn't happening.

A way to test this is to comment out the body of the for-await-lof loop in TransactionResponse:

    for await (const { statusChange } of subscription) {
//      if (statusChange.type !== 'SubmittedStatus') {
//        break;
//      }
    }

Then run any test that sends a transaction and awaits it to be executed (e.g. a contract call). If everything was working properly, this loop would close due to the connection closing.

I already tried fixing it (see comment). It worked, but I had to revert it because a ReadableStream isn't an AsyncIterable in most browsers so for-await-of can't be used on it. This, however, had a benefit: when a connection closes and for-await-of is being used, everything works as expected. So there might be some missing logic in the subscription implementation which doesn't close the stream.

Note that this isn't a pressing issue because we are always breaking the for-await-of loops that are consuming these subscriptions properly and no infinite loops or hangs happen because we aren't relying on the connection being closed by the node.

@arboleya
Copy link
Member

This issue description looks contradictory to me, so much so that I can't reconcile that you are filing a bug and simultaneously saying it's not a pressing issue for the same PR that is still open. If there is an issue with that PR, that should be fixed over there before that gets merged, and this issue is unnecessary.

@nedsalk
Copy link
Contributor Author

nedsalk commented Jan 11, 2024

I already spent quite some time trying to get this to work, fix it in that PR and not file this issue. Unfortunately, I couldn't get to the root of the problem. With filing this issue I wanted to unblock #1615 because this bug is not tied to introducing submitAndAwait per se, but to the underlying infrastructure and how we're using the streams API. Any SSE connection that's read via the streams API and that gets closed by the server isn't registered as closed by that same streams API.

@arboleya
Copy link
Member

Are you saying this issue was introduced after #1374 instead of #1615?

@nedsalk
Copy link
Contributor Author

nedsalk commented Jan 11, 2024

Correct. I'll try resolving this specific issue but we can also specify a timeout for subscriptions as is suggested by the vm client team, in which case an infinite loop would never be possible because we'd be closing the stream ourselves after that timeout. It wouldn't directly solve this issue, but this issue then should never happen because we'd close the stream ourselves at one point which'd end the stream correctly with a {value: undefined, done: true}.

@nedsalk nedsalk self-assigned this Jan 11, 2024
@nedsalk nedsalk added this to the 1 - Salamander milestone Jan 11, 2024
@arboleya
Copy link
Member

Then, I'd say we should pause #1615 and address this matter first. There's no harm in delaying the submitAndAwait, especially considering it'd also make the use of subscriptions more prevalent.

@nedsalk
Copy link
Contributor Author

nedsalk commented Jan 11, 2024

Well, it seems a good night's sleep was needed for this one. This issue is solved in #1615 (comment).

@nedsalk nedsalk closed this as completed Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is a bug
Projects
None yet
Development

No branches or pull requests

2 participants