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): fix race condition in process.subscribe that made the client crash #702

Merged
merged 2 commits into from
Nov 29, 2023

Conversation

kevin-dp
Copy link
Contributor

This PR fixes the race condition described in #426.
The problem was that process.subscribe was creating a promise, storing it, making an async call, fetching the promise and returning it. But, depending on the order in which messages are received from Electric, the async call may resolve after the subscription was already delivered and hence the promise had already been deleted. The fix consists of storing the promise in a local variable inside process.subscribe such that it doesn't need to fetch the promise after the async call.

…g it from the subscriptionNotifiers object. Also added a regression test.
Copy link

linear bot commented Nov 28, 2023

VAX-1006 Possible race condition in `process.subscribe()`

We sometimes are encountering an issue in the subscribe call. The subscription notifier doesn't have the subscription id after the client.subscribe() is finished. While debugging we noticed that when this happens it's because the subscription data message is received before client.subscribe() finishes. So when trying to read the promise from the subscription notifiers it's undefined.

The subscribe function

This is what triggers the websocket communication that can end up deleting the subscriptionId before finishing.

await this.client.subscribe(subId, shapeReqs)

Here is where we get the undefined exception.

synced: this.subscriptionNotifiers[subId].promise,

Here is where the subscription is being removed.

delete this.subscriptionNotifiers[subsData.subscriptionId] // GC the notifiers for this subscription ID

Would it make sense to check after client.subscribe() if the subscription exists? Or is that something that shouldn't happen?

When everything works fine, during client.subscribe() WebSocket only receives SatSubsResp. And after that the rest of the subscription messages are received (SatSubsDataBegin, ..., SatSubsDataEnd). When it fails this messages are received while the client.subscribe() is running.

Copy link
Contributor

@magnetised magnetised left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@balegas balegas left a comment

Choose a reason for hiding this comment

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

Looks good. Before merging, please do a sanity check of starting the linearlite example and see if there are no unexpected exceptions on server restarts. I'm a bit paranoid with that :D

@kevin-dp kevin-dp merged commit 3ed5469 into main Nov 29, 2023
6 checks passed
@kevin-dp kevin-dp deleted the kevindp/vax-1006-race-condition-in-process-subscribe branch November 29, 2023 10:55
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.

3 participants