-
Notifications
You must be signed in to change notification settings - Fork 28.4k
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
Fix continuation for configured default shell not used when reconnecting to Codespace #181832
Conversation
… to resolve, e.g. codespaces
this._profilesReadyPromise = this._remoteAgentService.getEnvironment() | ||
.then(() => { | ||
// Wait up to 20 seconds for profiles to be ready so it's assured that we know the actual | ||
// default terminal before launching the first terminal. This isn't expected to ever take | ||
// this long. | ||
this._profilesReadyBarrier = new AutoOpenBarrier(20000); | ||
return this._profilesReadyBarrier.wait().then(() => { }); | ||
}); |
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.
I lack the context why this barrier was needed before, maybe is not needed anymore after #175844 and we can remove it or maybe we should always race primaryBackend.getProfiles
call like Promise.race([primaryBackend.getProfiles, timeout(20000)])
?
With this change we start the barrier after the remote have been completely resolved, before that vscode is in Opening remote...
state waiting for the extension RemoteAuthorityResolver to finish
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.
We could maybe remove it, but I'm a little scared to tbh 🙂
this._profilesReadyPromise = this._remoteAgentService.getEnvironment() | ||
.then(() => { | ||
// Wait up to 20 seconds for profiles to be ready so it's assured that we know the actual | ||
// default terminal before launching the first terminal. This isn't expected to ever take | ||
// this long. | ||
this._profilesReadyBarrier = new AutoOpenBarrier(20000); | ||
return this._profilesReadyBarrier.wait().then(() => { }); | ||
}); |
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.
We could maybe remove it, but I'm a little scared to tbh 🙂
this._profilesReadyPromise = this._remoteAgentService.getEnvironment() | ||
.then(() => { | ||
// Wait up to 20 seconds for profiles to be ready so it's assured that we know the actual | ||
// default terminal before launching the first terminal. This isn't expected to ever take | ||
// this long. | ||
this._profilesReadyBarrier = new AutoOpenBarrier(20000); | ||
return this._profilesReadyBarrier.wait().then(() => { }); | ||
}); |
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.
@roblourens I don't think we end up resolving profiles for the ssh extension's special terminal but please ping me if ssh connecting with local servers has problems when this is merged.
Fix continuation when the remote takes more than 20s to resolve, e.g. codespaces
Fixes #175107
Fixes #181515