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

feat: no upgrade #1645

Merged
merged 4 commits into from
Mar 19, 2024
Merged

feat: no upgrade #1645

merged 4 commits into from
Mar 19, 2024

Conversation

owenpearson
Copy link
Member

@owenpearson owenpearson commented Feb 27, 2024

Jira links:

removes XHRStreaming transport, replaces upgrade mechanism with a websocket + base fallback transport mechanism whereby no two transports may be simultaneously connected.

@github-actions github-actions bot temporarily deployed to staging/pull/1645/features February 27, 2024 17:35 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1645/bundle-report February 27, 2024 17:36 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1645/typedoc February 27, 2024 17:36 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1645/features February 27, 2024 17:39 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1645/typedoc February 27, 2024 17:40 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1645/bundle-report February 27, 2024 17:40 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1645/features February 28, 2024 01:22 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1645/bundle-report February 28, 2024 01:23 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1645/typedoc February 28, 2024 01:23 Inactive
@owenpearson owenpearson marked this pull request as ready for review February 28, 2024 09:44
@github-actions github-actions bot temporarily deployed to staging/pull/1645/features February 28, 2024 16:08 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1645/bundle-report February 28, 2024 16:08 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1645/typedoc February 28, 2024 16:08 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1645/features February 28, 2024 17:37 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1645/typedoc February 28, 2024 17:38 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1645/bundle-report February 28, 2024 17:38 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1645/features February 28, 2024 17:40 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1645/bundle-report February 28, 2024 17:40 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1645/typedoc February 28, 2024 17:40 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1645/features February 29, 2024 00:18 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1645/features March 7, 2024 15:48 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1645/bundle-report March 7, 2024 15:48 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1645/typedoc March 7, 2024 15:48 Inactive
@owenpearson
Copy link
Member Author

@SimonWoolf I've addressed the feedback in #1645 (review) by setting the value of abandonedWebSocket and webSocketCheckResult upon successful websocket connectivity result in connectImpl

@SimonWoolf
Copy link
Member

I've addressed the feedback in #1645 (review) by setting the value of abandonedWebSocket and webSocketCheckResult upon successful websocket connectivity result in connectImpl

that's within a transportPreference === this.baseTransport check -- what if we don't have a baseTransport, eg someone's starting the lib with transports: ['web_socket']? then we still set abandonedWeboscket=true if the giveUpTimer expires, but nothing ever sets it to false again?

@github-actions github-actions bot temporarily deployed to staging/pull/1645/features March 11, 2024 18:16 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1645/bundle-report March 11, 2024 18:16 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1645/typedoc March 11, 2024 18:17 Inactive
@owenpearson
Copy link
Member Author

@SimonWoolf Good point, I've made it so that abandonedWebSocket is only set to true if there's a base transport to fall back to.

@lawrence-forooghian
Copy link
Collaborator

@owenpearson just a heads up that depending on which PR gets merged first you might need to also update the migration guide (see #1670)

@owenpearson owenpearson merged commit af0f3f1 into integration/v2 Mar 19, 2024
12 checks passed
@owenpearson owenpearson deleted the no-upgrade-v2 branch March 19, 2024 16:18
VeskeR added a commit that referenced this pull request Sep 6, 2024
…ycle

Fixes a regression introduced in ably-js v2 in the "no upgrade" PR [1].
Under certain network conditions (internet down) a race condition could
occur in the connection manager. Specifically, websocket connectivity
check in the `startWebSocketSlowTimer` function needed to set the
`wsCheckResult` flag to `false` before the timer was cleared by other
code branches. If that happened, it then caused an endless loop of
websocket connection retries, even after the network conditions
stabilized. A websocket transport would successfully open but then be
immediately disposed due to the `wsCheckResult` flag still being set to
`false`, without any mechanism to reset it in this scenario.

Upon closer look, it makes sense that the websocket connection health
flags (`wsCheckResult` and `abandonedWebSocket`) should be reset when
attempting a new websocket connection. The previous solution did not
explicitly reset these flags in the `connectWs` function, leading to the
described race condition and endless loop. Thus, `wsCheckResult` and
`abandonedWebSocket` are now reset to their neutral values upon entering
the `connectWs` function.

Resolves #1844

[1] #1645
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants