Consolidate local and remote terminal implementations #118252
Merged
Conversation
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
The hope is to reuse ptyHostService for local and remote
This will give a little more time to reattach after a crash Part of #118241
This is a no-op in local so I've put everything into remote terminal service and channel.
This is likely the cause of several terminal test issues we've been seeing, especially on remote. When process creation becomes async, the process seems to hang around if the terminal was disposed during the process creation. Another thing to consider if env var relaunches messing with tests.
|
Pushed a change to cancel createProcess if the terminal is disposed mid way through, I think this is why the test was failing. If there are still problems disabling env var relaunch seems like a good idea to reduce flakiness. |
Because of the create/start changes, RemotePty will no longer be initialized before the remote env is started which means there are no preconnection terminals anymore. This means that the terminal will not accept focus or input before the remote connection is ready.
This reverts commit 7bcec04.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
High level changes:
terminal.integrated.environmentChangesRelaunchsettingHere is the final layout for both local and remote:
Image source: pty_service.zip
Fixes #116467
Fixes #118241
Fixes #106354
Fixes #118454
Fixes #118543
Integration test flakiness issues:
Fixes #96057
Fixes #101911
Fixes #117370
It's possible web tests are improved as well but I'll defer that to another PR when remote flakiness is confirmed to be fixed.