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

Remove unncessary use of RPCProtocol. Fixes #11970 #11972

Merged
merged 1 commit into from
Jan 9, 2023

Conversation

tsmaeder
Copy link
Contributor

@tsmaeder tsmaeder commented Dec 8, 2022

Signed-off-by: Thomas Mäder t.s.maeder@gmail.com

What it does

Fixes #11970
Removes the unnecessary use of a RPCProtocol instance to communicate with shell processes in terminals.

Contributed on behalf of ST Microelectronics

How to test

Open terminals and make sure they work as usual. Tested on Windows 11. It might be interesting to test this on OSX and Linux. I've made sure performance is not significantly affected for large, fast output. For example, you can output the contents of large files in the terminal.

Review checklist

Reminder for reviewers

Contributed on behalf of ST Microelectronics

Signed-off-by: Thomas Mäder <t.s.maeder@gmail.com>
Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM otherwise.

Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code makes sense to me, and terminals on Windows still work.

@paul-marechal paul-marechal merged commit b50e840 into eclipse-theia:master Jan 9, 2023
@vince-fugnitto
Copy link
Member

I believe this change may be resulting in our CI failing:

PR:

image

Master:

image

All subsequent builds are now failing due to a task test:

@theia/task:   1 failing
@theia/task:   1) Task server / back-end
@theia/task:        task running in terminal - expected data is received from the terminal ws server:
@theia/task:      Error: Timeout of 10000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/runner/work/theia/theia/packages/task/lib/node/task-server.slow-spec.js)
@theia/task:       at listOnTimeout (node:internal/timers:559:17)
@theia/task:       at processTimers (node:internal/timers:502:7)

@vince-fugnitto
Copy link
Member

@tsmaeder do you have time to investigate a fix for the test, else we may need to revert the changes for the time being.

@vince-fugnitto vince-fugnitto added the terminal issues related to the terminal label Jan 10, 2023
paul-marechal added a commit that referenced this pull request Jan 10, 2023
One of the task test cases relies on the protocol used by terminals
to communicate data, but the protocol has been updated in #11972.
paul-marechal added a commit that referenced this pull request Jan 10, 2023
One of the task test cases relies on the protocol used by terminals
to communicate data, but the protocol has been updated in #11972.
@paul-marechal
Copy link
Member

CI failure fixed by #12054.

@vince-fugnitto vince-fugnitto added this to the 1.34.0 milestone Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
terminal issues related to the terminal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Terminal Connections use Unnecessary RPC Protocol
3 participants