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

Migrate waitForConnectionToBeUsable() to react to pushed connection event state changes #882

Merged
merged 26 commits into from
Jan 10, 2025

Conversation

jlrobins
Copy link
Contributor

@jlrobins jlrobins commented Jan 9, 2025

Summary of Changes

  • Rename waitForConnectionToBeUsable() to waitForConnectionToBeStable(), in that it may not always return a happy connection (say, if cclioud auth is bad, or direct connection cannot connect, etc., or if it times out).
  • Migrate waitForConnectionToBeStable() from a polling GET loop to reacting against connection state change events pushed by sidecar over websocket.
  • Do so through construction of a new singleton class ConnectionStateWatcher which consumes the sidecar -> workspace websocket messages describing changes to connection states, keeps track of the most recently observed connection state, and offers an API for waitForConnectionToBeStable() to use to pause just the right amount of time until sidecar pushes a stable-state Connection description for the connection id in question that waitForConnectionToBeStable() was called about. Even works if the websocket event with the new stable state precedes the call to waitForConnectionToBeStable(), as is the curious case with CCloud connection creation and the byzantine authentication flow, due to ConnectionStateWatcher caching the most recently received update for a connection and will return it to waitForConnectionToBeStable() if it meets the 'stable state' criteria for the connection type (sadly complicated by how sidecar describes a ccloud connection in the progress of authorization vs a direct connection not yet verified, originally expressed in a large case statement embedded in waitForConnectionToBeStable()'s while loop, now expressed in the functions in new module connectionStatusUtils.ts.
  • Closes Handle websocket-pushed connection status change events instead of polling #851. End result is that we skip polling loop with a fixed interval pause in favor of reacting immediately sidecar pushing updated ccloud or direction connection state to us, making us react as fast as possible.

Any additional details or context that should be provided?

  • Next up in this theme: killing off the ccloud auth status interval polling. Will be easy to reexpress on top of the new ConnectionStateWatcher, which now knows exactly what the ccloud auth status poller is interested in.

Pull request checklist

Please check if your PR fulfills the following (if applicable):

Tests
  • Added new
  • Updated existing
  • Deleted existing
Other
  • All new disposables (event listeners, views, channels, etc.) collected as for eventual cleanup?
  • Does anything in this PR need to be mentioned in the user-facing CHANGELOG or README?
  • Have you validated this change locally by packaging and installing the extension .vsix file?
    gulp clicktest

… push the connection state filtering down into a lambda passed into ConnectionStateWatcher.waitForConnectionUpdate().
…ts about connection from websocket, not polling so as to be able to react immediately.
@jlrobins jlrobins added this to the Websocket Subsystem milestone Jan 9, 2025
@jlrobins jlrobins changed the title Wensocket connection change event handling Migrate waitForConnectionToBeUsable() to react to pushed connection event state changes Jan 9, 2025
@@ -1 +1 @@
v0.134.1
v0.139.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need fix for ccloud connection java.time.Instant serialization-over-websocket.

@jlrobins jlrobins marked this pull request as ready for review January 10, 2025 03:37
@jlrobins jlrobins requested a review from a team as a code owner January 10, 2025 03:37
Copy link
Contributor

@shouples shouples left a comment

Choose a reason for hiding this comment

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

Overall I'm good with the changes, and mostly had minor suggestions/questions regarding logging and documentation, but the only change I think we need is to put connectionLoading.fire(id) back since it looks like we no longer show the loading~spin icon when a connection is in a non-stable state -- should that live inside waitForConnectionUpdate, or in the ConnectionStateWatcher itself?

src/sidecar/connectionStatusUtils.ts Outdated Show resolved Hide resolved
src/sidecar/connectionStatusUtils.ts Outdated Show resolved Hide resolved
src/sidecar/connections.ts Outdated Show resolved Hide resolved
src/sidecar/connections.ts Outdated Show resolved Hide resolved
src/sidecar/connections.ts Outdated Show resolved Hide resolved
src/sidecar/connections.ts Outdated Show resolved Hide resolved
src/sidecar/connections.ts Outdated Show resolved Hide resolved
src/sidecar/connections.ts Outdated Show resolved Hide resolved
src/sidecar/connections.ts Outdated Show resolved Hide resolved
@jlrobins jlrobins requested a review from shouples January 10, 2025 17:23
@jlrobins jlrobins requested a review from shouples January 10, 2025 18:03
Copy link
Contributor

@shouples shouples left a comment

Choose a reason for hiding this comment

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

@jlrobins and I still have some lingering edge cases to hammer out in follow-on branches since this overall performance improvement floated some race conditions we previously didn't have to worry about -- future us will deal with it next week!

@jlrobins jlrobins merged commit 3824618 into main Jan 10, 2025
2 checks passed
@jlrobins jlrobins deleted the wensocket_connection_change_event_handling branch January 10, 2025 21:14
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.

Handle websocket-pushed connection status change events instead of polling
2 participants