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

Fix continuation for configured default shell not used when reconnecting to Codespace #181832

Merged
merged 3 commits into from Aug 4, 2023

Conversation

jeanp413
Copy link
Contributor

@jeanp413 jeanp413 commented May 8, 2023

Fix continuation when the remote takes more than 20s to resolve, e.g. codespaces

Fixes #175107
Fixes #181515

Comment on lines +71 to +78
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(() => { });
});
Copy link
Contributor Author

@jeanp413 jeanp413 May 8, 2023

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

Copy link
Member

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 🙂

Comment on lines +71 to +78
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(() => { });
});
Copy link
Member

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 🙂

Comment on lines +71 to +78
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(() => { });
});
Copy link
Member

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.

@Tyriar Tyriar added this to the August 2023 milestone Aug 4, 2023
@Tyriar Tyriar merged commit 2f951c9 into microsoft:main Aug 4, 2023
6 checks passed
@jeanp413 jeanp413 deleted the fix-175107-2 branch August 5, 2023 02:16
@github-actions github-actions bot locked and limited conversation to collaborators Sep 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

First terminal does not follow default. Configured default shell not used when reconnecting to Codespace
3 participants