Implement background reconnection #2284
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -689,7 +689,7 @@ index 3715cbb8e6ee41c3d9b5090918d243b723ae2d00..c65de8ad37e727d66da97a8f8b170cbc | |
| - | ||
| - | ||
| diff --git a/src/vs/platform/remote/common/remoteAgentConnection.ts b/src/vs/platform/remote/common/remoteAgentConnection.ts | ||
| index 18d3d04fd20335975293e37b3b641120dd92da20..a06f20ece490dba5d88b41268cddaaf6b2f6b64a 100644 | ||
| --- a/src/vs/platform/remote/common/remoteAgentConnection.ts | ||
| +++ b/src/vs/platform/remote/common/remoteAgentConnection.ts | ||
| @@ -92,7 +92,7 @@ async function connectToRemoteExtensionHostAgent(options: ISimpleConnectionOptio | ||
|
|
@@ -701,6 +701,82 @@ index 18d3d04fd20335975293e37b3b641120dd92da20..4e49f9d63623da6c84624144765f76ec | |
| (err: any, socket: ISocket | undefined) => { | ||
| if (err || !socket) { | ||
| options.logService.error(`${logPrefix} socketFactory.connect() failed. Error:`); | ||
| @@ -331,12 +331,16 @@ export const enum PersistentConnectionEventType { | ||
| } | ||
| export class ConnectionLostEvent { | ||
| public readonly type = PersistentConnectionEventType.ConnectionLost; | ||
| + constructor( | ||
| + public readonly connectionAttempt?: number | ||
| + ) { } | ||
| } | ||
| export class ReconnectionWaitEvent { | ||
| public readonly type = PersistentConnectionEventType.ReconnectionWait; | ||
| constructor( | ||
| public readonly durationSeconds: number, | ||
| - private readonly cancellableTimer: CancelablePromise<void> | ||
| + private readonly cancellableTimer: CancelablePromise<void>, | ||
| + public readonly connectionAttempt?: number, | ||
| ) { } | ||
|
|
||
| public skipWait(): void { | ||
| @@ -345,12 +349,21 @@ export class ReconnectionWaitEvent { | ||
| } | ||
| export class ReconnectionRunningEvent { | ||
| public readonly type = PersistentConnectionEventType.ReconnectionRunning; | ||
| + constructor( | ||
| + public readonly connectionAttempt?: number | ||
| + ) { } | ||
| } | ||
| export class ConnectionGainEvent { | ||
| public readonly type = PersistentConnectionEventType.ConnectionGain; | ||
| + constructor( | ||
| + public readonly connectionAttempt?: number | ||
| + ) { } | ||
| } | ||
| export class ReconnectionPermanentFailureEvent { | ||
| public readonly type = PersistentConnectionEventType.ReconnectionPermanentFailure; | ||
| + constructor( | ||
| + public readonly connectionAttempt?: number | ||
| + ) { } | ||
| } | ||
| export type PersistenConnectionEvent = ConnectionGainEvent | ConnectionLostEvent | ReconnectionWaitEvent | ReconnectionRunningEvent | ReconnectionPermanentFailureEvent; | ||
|
|
||
| @@ -411,8 +424,9 @@ abstract class PersistentConnection extends Disposable { | ||
| } | ||
| 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()); | ||
| + this._onDidStateChange.fire(new ConnectionLostEvent(0)); | ||
| const TIMES = [5, 5, 10, 10, 10, 10, 10, 30]; | ||
| + | ||
| const disconnectStartTime = Date.now(); | ||
| let attempt = -1; | ||
| do { | ||
| @@ -420,7 +434,7 @@ abstract class PersistentConnection extends Disposable { | ||
| const waitTime = (attempt < TIMES.length ? TIMES[attempt] : TIMES[TIMES.length - 1]); | ||
| try { | ||
| const sleepPromise = sleep(waitTime); | ||
| - this._onDidStateChange.fire(new ReconnectionWaitEvent(waitTime, sleepPromise)); | ||
| + this._onDidStateChange.fire(new ReconnectionWaitEvent(waitTime, sleepPromise, attempt)); | ||
|
|
||
| this._options.logService.info(`${logPrefix} waiting for ${waitTime} seconds before reconnecting...`); | ||
| try { | ||
| @@ -433,13 +447,13 @@ abstract class PersistentConnection extends Disposable { | ||
| } | ||
|
|
||
| // connection was lost, let's try to re-establish it | ||
| - this._onDidStateChange.fire(new ReconnectionRunningEvent()); | ||
| + this._onDidStateChange.fire(new ReconnectionRunningEvent(attempt)); | ||
| this._options.logService.info(`${logPrefix} resolving connection...`); | ||
| const simpleOptions = await resolveConnectionOptions(this._options, this.reconnectionToken, this.protocol); | ||
| this._options.logService.info(`${logPrefix} connecting to ${simpleOptions.host}:${simpleOptions.port}...`); | ||
| await connectWithTimeLimit(simpleOptions.logService, this._reconnect(simpleOptions), RECONNECT_TIMEOUT); | ||
| this._options.logService.info(`${logPrefix} reconnected!`); | ||
| - this._onDidStateChange.fire(new ConnectionGainEvent()); | ||
| + this._onDidStateChange.fire(new ConnectionGainEvent(attempt)); | ||
|
|
||
| break; | ||
| } catch (err) { | ||
| diff --git a/src/vs/platform/storage/browser/storageService.ts b/src/vs/platform/storage/browser/storageService.ts | ||
| index ab3fd347b69f8a3d9b96e706cd87c911b8ffed6b..9d351037b577f9f1edfd18ae9b3c48a211f4467f 100644 | ||
| --- a/src/vs/platform/storage/browser/storageService.ts | ||
|
|
@@ -3199,6 +3275,84 @@ index 94e7e7a4bac154c45078a1b5034e50634a7a43af..8164200dcef1efbc65b50eef9c270af3 | |
| this._filenameKey.set(value ? basename(value) : null); | ||
| this._dirnameKey.set(value ? dirname(value).fsPath : null); | ||
| this._pathKey.set(value ? value.fsPath : null); | ||
| diff --git a/src/vs/workbench/contrib/remote/browser/remote.ts b/src/vs/workbench/contrib/remote/browser/remote.ts | ||
| index 98573a206f14928fc3fdf18fe927cb75034e4ad1..1430666aa94f941bda086df503fec8b35aa2b25f 100644 | ||
| --- a/src/vs/workbench/contrib/remote/browser/remote.ts | ||
| +++ b/src/vs/workbench/contrib/remote/browser/remote.ts | ||
| @@ -730,6 +730,7 @@ class RemoteAgentConnectionStatusListener implements IWorkbenchContribution { | ||
| @IContextKeyService contextKeyService: IContextKeyService | ||
| ) { | ||
| const connection = remoteAgentService.getConnection(); | ||
| + const SHOW_POPUP_ON_ATTEMPT = 2 // aka third attempt | ||
| if (connection) { | ||
| let visibleProgress: VisibleProgress | null = null; | ||
| let lastLocation: ProgressLocation.Dialog | ProgressLocation.Notification | null = null; | ||
| @@ -793,33 +794,47 @@ class RemoteAgentConnectionStatusListener implements IWorkbenchContribution { | ||
| disposableListener.dispose(); | ||
| disposableListener = null; | ||
| } | ||
| + let suppressPopup = (typeof e.connectionAttempt == 'number' && e.connectionAttempt < SHOW_POPUP_ON_ATTEMPT) | ||
| + let forceDialog = (typeof e.connectionAttempt == 'number' && e.connectionAttempt == SHOW_POPUP_ON_ATTEMPT) | ||
| switch (e.type) { | ||
| case PersistentConnectionEventType.ConnectionLost: | ||
| - 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 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 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 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! |
||
| + } else { | ||
| + if (!visibleProgress) { | ||
| + visibleProgress = showProgress(ProgressLocation.Dialog, [reconnectButton, reloadButton]); | ||
| + } | ||
| + visibleProgress.report(nls.localize('connectionLost', "Connection Lost")); | ||
| } | ||
| - visibleProgress.report(nls.localize('connectionLost', "Connection Lost")); | ||
| break; | ||
| case PersistentConnectionEventType.ReconnectionWait: | ||
| reconnectWaitEvent = e; | ||
| - visibleProgress = showProgress(lastLocation || ProgressLocation.Notification, [reconnectButton, reloadButton]); | ||
| - visibleProgress.startTimer(Date.now() + 1000 * e.durationSeconds); | ||
| + if (suppressPopup) { | ||
| + hideProgress() | ||
| + } else { | ||
| + const location = forceDialog ? ProgressLocation.Dialog : (lastLocation || ProgressLocation.Notification) | ||
| + visibleProgress = showProgress(location, [reconnectButton, reloadButton]); | ||
| + visibleProgress.startTimer(Date.now() + 1000 * e.durationSeconds); | ||
| + } | ||
| break; | ||
| case PersistentConnectionEventType.ReconnectionRunning: | ||
| - visibleProgress = showProgress(lastLocation || ProgressLocation.Notification, [reloadButton]); | ||
| - visibleProgress.report(nls.localize('reconnectionRunning', "Attempting to reconnect...")); | ||
| - | ||
| - // Register to listen for quick input is opened | ||
| - disposableListener = contextKeyService.onDidChangeContext((contextKeyChangeEvent) => { | ||
| - const reconnectInteraction = new Set<string>([inQuickPickContextKeyValue]); | ||
| - if (contextKeyChangeEvent.affectsSome(reconnectInteraction)) { | ||
| - // Need to move from dialog if being shown and user needs to type in a prompt | ||
| - if (lastLocation === ProgressLocation.Dialog && visibleProgress !== null) { | ||
| - visibleProgress = showProgress(ProgressLocation.Notification, [reloadButton], visibleProgress.lastReport); | ||
| + if (suppressPopup) { | ||
| + hideProgress() | ||
| + } else { | ||
| + visibleProgress = showProgress(lastLocation || ProgressLocation.Notification, [reloadButton]); | ||
| + visibleProgress.report(nls.localize('reconnectionRunning', "Attempting to reconnect...")); | ||
| + | ||
| + // Register to listen for quick input is opened | ||
| + disposableListener = contextKeyService.onDidChangeContext((contextKeyChangeEvent) => { | ||
| + const reconnectInteraction = new Set<string>([inQuickPickContextKeyValue]); | ||
| + if (contextKeyChangeEvent.affectsSome(reconnectInteraction)) { | ||
| + // Need to move from dialog if being shown and user needs to type in a prompt | ||
| + if (lastLocation === ProgressLocation.Dialog && visibleProgress !== null) { | ||
| + visibleProgress = showProgress(ProgressLocation.Notification, [reloadButton], visibleProgress.lastReport); | ||
| + } | ||
| } | ||
| - } | ||
| - }); | ||
| - | ||
| + }); | ||
| + } | ||
| break; | ||
| case PersistentConnectionEventType.ReconnectionPermanentFailure: | ||
| hideProgress(); | ||
| diff --git a/src/vs/workbench/contrib/scm/browser/media/scm.css b/src/vs/workbench/contrib/scm/browser/media/scm.css | ||
| index ac44ad3bae428def66e22fe9cc1c54648d429f6b..faa63023c4c586b51fa3c2a48ff3641b9cb0e145 100644 | ||
| --- a/src/vs/workbench/contrib/scm/browser/media/scm.css | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nhooyr @mgmachado @code-asher
a search in the vscode repo indicates 3 subscribers to
PersistentConnectionEventType:RemoteAgentConnectionStatusListener- github linkRemoteExtensionHostEnvironmentUpdater- github linkExtensionService- github linkThe
ExtensionServicedoes act on theConnectionLostevent - so that it can_clearResolvedAuthority. TheRemoteExtensionHostEnvironmentUpdateris only interested in theConnectionGainevent - also related to the remote authority.An alternative approach could be to modify
RemoteAgentConnectionStatusListenerto 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).