Implement background reconnection #2284
Conversation
There was a problem hiding this comment.
Thank you for contributing @mgmachado, this looks really awesome!
🔥
Going to need approval from @code-asher
| } | ||
| const logPrefix = commonLogPrefix(this._connectionType, this.reconnectionToken, true); | ||
| this._options.logService.info(`${logPrefix} starting reconnecting loop. You can get more information with the trace log level.`); | ||
| - this._onDidStateChange.fire(new ConnectionLostEvent()); |
There was a problem hiding this comment.
Only concern I have is whether it's ok to not fire this here. Maybe it prevents VS Code from trying to do things on the connection?
But I'm not sure. Just something to note. cc @code-asher
There was a problem hiding this comment.
@nhooyr @mgmachado @code-asher
a search in the vscode repo indicates 3 subscribers to PersistentConnectionEventType:
RemoteAgentConnectionStatusListener- github linkRemoteExtensionHostEnvironmentUpdater- github linkExtensionService- github link
The ExtensionService does act on the ConnectionLost event - so that it can _clearResolvedAuthority. The RemoteExtensionHostEnvironmentUpdater is only interested in the ConnectionGain event - also related to the remote authority.
An alternative approach could be to modify RemoteAgentConnectionStatusListener to handle the events differently. In the same file, it seems like changing VisibleProgress with some sort of delayed activation might be cleaner (after a name change perhaps).
There was a problem hiding this comment.
Thank you for the PR!
To echo @nhooyr's comment, there can be some unintended consequences if we modify the underlying events.
For example, we rely on these events for our Node proxy service and this could break the service because we need to re-establish the proxy when the connection goes down and comes back up but if that happens in less than three attempts we won't get the necessary events.
Instead we should modify the popup itself, I think. It looks like it's in /lib/vscode/src/vs/workbench/contrib/remote/browser/remote.ts.
Potential implementation: keep a counter that increments each time ReconnectionWait happens and resets to zero every time ConnectionGain happens then use that to determine whether to show the dialogs.
Another potential solution is to change ProgressLocation.Dialog to ProgressLocation.Notification so it appears in the bottom right corner instead of in the middle where it hijacks everything. The advantage here is that it lets the user be more aware of when the connection goes down while still being able to use the editor normally but there's already a persistent connection status in the bottom left so it's probably not a big deal. I'm good either way.
|
@choyrim @nhooyr @code-asher thank you for the feedback. I agree that modifying the behavior within the popup is a better approach than suppressing the events. Should I convert this PR to draft while I work on the suggestions posted here? |
tbh, I don't really have a preference. I believe the conversion to draft is a somewhat new feature. I suppose it makes sense in this case to indicate this is a WIP. |
|
Hey code server devs, I implemented a better solution, but it caused a merge check to fail. I'm working on resolving that - it's the one that tests that the release standalone image can install python and jupyter extensions correctly. |
I believe that failure is unrelated. Feel free to push your changes up and just make sure you're rebased on |
…without_suppressing_events Fix/suppress reconnect popups without suppressing events
|
The suggestion to convert The behavior I've implemented now is: distraction free reconnects the first 2 times, and a Dialog popup at the third attempt to bring the re-connection failure to the user's attention, without suppressing the firing of the connection events. |
|
@nhooyr @code-asher do you still have concerns about this PR after the refactoring? or do you need us to resolve the latest conflicts before you can review? Let us know. |
|
No this looks great! I'll test it today and if all works well, it'll be in the next release! |
| - if (!visibleProgress) { | ||
| - visibleProgress = showProgress(ProgressLocation.Dialog, [reconnectButton, reloadButton]); | ||
| + if (suppressPopup) { | ||
| + hideProgress() |
There was a problem hiding this comment.
Why are these calls necessary? Wouldn't we not have made progress visible if the connection attempt was lower than SHOW_POPUP_ON_ATTEMPT?
There was a problem hiding this comment.
@nhooyr Did you mean "Wouldn't we not have made progress visible if the connection attempt was greater than SHOW_POPUP_ON_ATTEMPT?" Then yes, these calls are redundant. The code could be simplified to
switch (e.type) {
case PersistentConnectionEventType.ConnectionLost:
hideProgress();
break;Or did you mean something else?
There was a problem hiding this comment.
Yes sorry, I didn't mean to add the extra not in there.
Yup in that case this looks good to me, will merge. I'll do a follow up commit later to simplify what I can.
Thanks again!
There was a problem hiding this comment.
Thanks a lot for the feedback and accepting the PR!
ae65c83 to
2a3608d
Compare
|
Other than that last comment, this looks great! Thanks for contributing @mgmachado @choyrim Greatly appreciated 🚀 |
Issue this PR solves: #1791
On a connection loss, the client attempts to reconnect up to twice in the background before displaying the reconnection pop-up.