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

expose pty terminal api to detect when it is explicitly killed by a user #130231

Closed
akosyakov opened this issue Aug 6, 2021 · 7 comments · Fixed by #152833 or #157653
Closed

expose pty terminal api to detect when it is explicitly killed by a user #130231

akosyakov opened this issue Aug 6, 2021 · 7 comments · Fixed by #152833 or #157653
Assignees
Labels
api api-finalization help wanted Issues identified as good community contribution opportunities insiders-released Patch has been released in VS Code Insiders on-release-notes Issue/pull request mentioned in release notes terminal Integrated terminal issues
Milestone

Comments

@akosyakov
Copy link
Contributor

akosyakov commented Aug 6, 2021

Right now there is only close method which is called by different user actions. I need to distinguish the case when the terminal is closed because the user explicitly requested closing of this terminal or because a user, for instance, close the window and disposure of the frontend terminal as well as the ext host terminal service call close. Maybe dispose is missing?

Context: I have long-running processes to which pty is connecting. Such processes should be stopped if a user requests explicit killing of frontend terminals, otherwise they should not be. But right now they got stopped whenever a user closes the window.

@akosyakov akosyakov changed the title export pty terminal api to detect when it is explicitly killed by a user expose pty terminal api to detect when it is explicitly killed by a user Aug 6, 2021
@Tyriar
Copy link
Member

Tyriar commented Aug 6, 2021

Does this work?

vscode/src/vs/vscode.d.ts

Lines 9680 to 9692 in 19959ec

/**
* Represents how a terminal exited.
*/
export interface TerminalExitStatus {
/**
* The exit code that a terminal exited with, it can have the following values:
* - Zero: the terminal process or custom execution succeeded.
* - Non-zero: the terminal process or custom execution failed.
* - `undefined`: the user forcibly closed the terminal or a custom execution exited
* without providing an exit code.
*/
readonly code: number | undefined;
}

@Tyriar Tyriar added the info-needed Issue requires more information from poster label Aug 6, 2021
@akosyakov
Copy link
Contributor Author

It is undefined for me in both cases, i.e. when I reload the page and forcefully kills the terminal.

@meganrogge meganrogge added terminal Integrated terminal issues terminal-triage and removed info-needed Issue requires more information from poster labels Oct 6, 2021
@Tyriar
Copy link
Member

Tyriar commented Oct 13, 2021

I believe we treat both these cases the same internally, I since there is the difference of the extension host going down when a window is closed or reloaded, take that into consideration as to whether you want to kill the long running process or not.

@Tyriar Tyriar added the *out-of-scope Posted issue is not in scope of VS Code label Oct 13, 2021
@jeanp413
Copy link
Contributor

Is this still out of scope? I have a hack in my extension as you suggested by it's broken now due to #152204 , which I'm sure it will be fixed, but having a way to differentiate when a user closes the terminal explicitly would be nice

@akosyakov
Copy link
Contributor Author

@Tyriar Would you be opened if we propose some API to distinguish between close caused by explicit user action to kill the terminal or by system (i.e. unload of extension host caused by window reload or close). As @jeanp413 mentioned we have code like that: https://github.com/gitpod-io/openvscode-server/blob/fa1ade7210e7bba9075dadf3d84a7f65ffcd3da1/extensions/gitpod-shared/src/features.ts#L963-L965 which got broke by last release, but generally we hear complaints from time to time that underlying jobs getting closed by window reloads. We would like to have more robust means than timeout.

@Tyriar
Copy link
Member

Tyriar commented Jun 16, 2022

Sure you can propose and I'll bring it to our API meeting to discuss, I guess something like TerminalExitStatus.source: 'process' | ... is what you're after?

@Tyriar Tyriar reopened this Jun 16, 2022
@Tyriar Tyriar added help wanted Issues identified as good community contribution opportunities api api-proposal and removed *out-of-scope Posted issue is not in scope of VS Code terminal-triage labels Jun 16, 2022
@Tyriar Tyriar added this to the Backlog milestone Jun 16, 2022
@Tyriar Tyriar modified the milestones: Backlog, July 2022 Jun 24, 2022
@VSCodeTriageBot VSCodeTriageBot added the unreleased Patch has not yet been released in VS Code Insiders label Jul 11, 2022
@Tyriar Tyriar removed this from the July 2022 milestone Jul 11, 2022
@Tyriar Tyriar added this to the August 2022 milestone Jul 11, 2022
@Tyriar Tyriar reopened this Jul 11, 2022
@VSCodeTriageBot VSCodeTriageBot removed the unreleased Patch has not yet been released in VS Code Insiders label Jul 11, 2022
Tyriar added a commit that referenced this issue Aug 9, 2022
@VSCodeTriageBot VSCodeTriageBot added unreleased Patch has not yet been released in VS Code Insiders insiders-released Patch has been released in VS Code Insiders and removed unreleased Patch has not yet been released in VS Code Insiders labels Aug 9, 2022
joyceerhl pushed a commit that referenced this issue Aug 10, 2022
@meganrogge meganrogge added the on-release-notes Issue/pull request mentioned in release notes label Aug 26, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Sep 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api api-finalization help wanted Issues identified as good community contribution opportunities insiders-released Patch has been released in VS Code Insiders on-release-notes Issue/pull request mentioned in release notes terminal Integrated terminal issues
Projects
None yet
6 participants
@Tyriar @akosyakov @jeanp413 @meganrogge @VSCodeTriageBot and others