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

Adopt utility process for pty host on Electron #182345

Merged
merged 11 commits into from
May 15, 2023
Merged

Conversation

Tyriar
Copy link
Member

@Tyriar Tyriar commented May 12, 2023

Part of #175335

@Tyriar Tyriar added this to the May 2023 milestone May 12, 2023
@Tyriar Tyriar self-assigned this May 12, 2023
@Tyriar Tyriar requested a review from bpasero May 15, 2023 14:13
@Tyriar Tyriar marked this pull request as ready for review May 15, 2023 14:13
@Tyriar
Copy link
Member Author

Tyriar commented May 15, 2023

@bpasero ready for review 🙂

bpasero
bpasero previously approved these changes May 15, 2023
Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

Performance related question: when is the PTY host starting, will a window trigger the request when a terminal is opened or do we eagerly spawn the PTY host?

I suggest we get this going in insiders to try it out, I did not do a deep review of the pieces but trust your expertise in this area.

One thing I saw when testing: a message [main 2023-05-15T15:58:20.775Z] ptyHost terminated unexpectedly with code 0 shows when I open a terminal, reload the window and then quit.

@Tyriar Tyriar force-pushed the tyriar/utility_ptyhost__2 branch from 25f6ada to a12c412 Compare May 15, 2023 21:02
@Tyriar Tyriar enabled auto-merge May 15, 2023 21:18
@Tyriar Tyriar merged commit c3c6a33 into main May 15, 2023
@Tyriar Tyriar deleted the tyriar/utility_ptyhost__2 branch May 15, 2023 23:49
@github-actions github-actions bot locked and limited conversation to collaborators Jun 30, 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.

4 participants