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

(FFM-1857) Streaming reconnect #58

Merged
merged 2 commits into from
Dec 14, 2021
Merged

Conversation

conormurray95
Copy link
Contributor

What
For our SDK's the desired functionality if we lose connection is to poll the data directly to collect any updates we missed then re-establish the stream. There were bugs in this area that this pr addresses.

Fixes

  1. When running the reconnect poll via the retrieve function it included logic only required on startup i.e. pushing a value to the c.initialized channel - this meant when called later nobody was listening on the channel and the thread would hang forever.
  2. We didn't check that the data poll succeeded before trying to restart the stream
  3. We didn't run the onDisconnect logic if the stream failed to start up - only if it disconnected, this meant if this path was hit the stream wouldn't start but c.streamConnected would still be set to true preventing any new streams starting to replace it
  4. The sse library we use has by default an ExponentialBackoff strategy where it continually tries to reconnect to the stream - because we have our own disconnect logic to poll then start a new stream we don't want this - otherwise we'd end up with 2 streams listening for the same events so we set it to StopBackOff strategy where it doesn't attempt to re-establish the connection and we can handle it
  5. In our restart logic c.streamConnect() was started as a goroutine - there was no need for this and it defeated the purpose of the rLock we used to control access to this block of code to prevent multiple threads trying to poll/start the stream
  6. Added extra logging around the stream disconnecting -> moving to polling mode-> back to streaming mode

Testing
Simulate the stream being lost and test that it reconnects after a successful poll, the simplest way to do this is turn off your wifi and wait 5 minutes

@CLAassistant
Copy link

CLAassistant commented Dec 6, 2021

CLA assistant check
All committers have signed the CLA.

@conormurray95 conormurray95 force-pushed the FFM-1857-streaming-reconnect branch 2 times, most recently from 3e4e846 to 510df99 Compare December 6, 2021 15:57
Copy link
Contributor

@davejohnston davejohnston left a comment

Choose a reason for hiding this comment

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

LGTM. One query about the connect method and whether we need to do anything to handle what it returns.

Copy link
Contributor

@davejohnston davejohnston left a comment

Choose a reason for hiding this comment

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

LGTM

@conormurray95 conormurray95 merged commit dfb6557 into main Dec 14, 2021
@erdirowlands erdirowlands deleted the FFM-1857-streaming-reconnect branch November 3, 2023 17:44
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.

None yet

3 participants