Skip to content
This repository has been archived by the owner on Oct 19, 2024. It is now read-only.

fix: fix re-subscription on websocket reconnect #2419

Merged
merged 2 commits into from
May 23, 2023
Merged

Conversation

gbrew
Copy link
Contributor

@gbrew gbrew commented May 15, 2023

Add an in-flight request for each re-subscription request to ensure that the response from the server with the new internal subscription id is handled.

Motivation

This change fixes issue 2418, which is an issue where websocket subscriptions can stop working after a websocket reconnect.

Solution

The underlying issue is that the re-subscription code did not have an in-flight request for the resubscription requests. They were issued properly, but the response from the server containing the new server-side subscription ID was then dropped as a result. This change adds the appropriate in-flight requests so the new server-side ID is handled properly and updated.

PR Checklist

  • [ x] Added Tests
  • Added Documentation
  • Breaking changes

Add an in-flight request for each re-subscription request to ensure
that the response from the server with the new internal subscription
id is handled.
@DaniPopes DaniPopes requested a review from prestwich May 15, 2023 04:30
@gbrew
Copy link
Contributor Author

gbrew commented May 15, 2023

Thanks for the review @prestwich , and thanks for all the work on reconnects!

I had a quick look at the failing CI tests, and they all seem likely related to a solc version change ( ethers-solc compile::tests::test_detect_version ), and unrelated to my change. Should I worry about making those pass or just ignore them? And is there anything else I should be doing to get this merged?

@prestwich
Copy link
Collaborator

Should I worry about making those pass or just ignore them? And is there anything else I should be doing to get this merged?

just ignore those 👍 I'll be doing a pass later today to merge things and hopefully get those addressed as well

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