fix #2961: a workaround to show output and title of short-lived tasks. #7776
Conversation
| widgetOptions: { area: 'bottom' }, | ||
| mode: widgetOpenMode, | ||
| taskInfo | ||
| }); |
There was a problem hiding this comment.
The formatting is done by Theia automatically.
| @@ -41,10 +41,10 @@ export abstract class BaseTerminalServer implements IBaseTerminalServer { | |||
| abstract create(options: IBaseTerminalServerOptions): Promise<number>; | |||
|
|
|||
| async attach(id: number): Promise<number> { | |||
| const term = this.processManager.get(id); | |||
| const terminal = this.processManager.get(id); | |||
There was a problem hiding this comment.
Align the naming convention in this file.
| @@ -997,24 +993,19 @@ export class TaskService implements TaskConfigurationClient { | |||
| terminal.sendText(selectedText); | |||
| } | |||
|
|
|||
| async attach(terminalId: number, taskId: number): Promise<void> { | |||
| // Get the list of all available running tasks. | |||
| const runningTasks: TaskInfo[] = await this.getRunningTasks(); | |||
There was a problem hiding this comment.
If no corresponding taskInfo is found, the attach function would end up with opening a widget labeled with something like Task: #23 rather than the label of the task, which is ugly.
There was a problem hiding this comment.
Thank you for taking the time to provide a workaround for the issue @a1994846931931.
Overall I see there are unnecessary changes and breakage which could be avoided.
I don't think we should mark the issue as fixed since it is only a workaround and the underlying issue is still present.
| @@ -177,8 +177,6 @@ export interface TaskIdentifier { | |||
| export interface TaskInfo { | |||
| /** internal unique task id */ | |||
| readonly taskId: number, | |||
| /** terminal id. Defined if task is run as a terminal process */ | |||
| readonly terminalId?: number, | |||
There was a problem hiding this comment.
Why are we removing the terminalId from the interface.
It is an unnecessary breaking change.
| export interface AttachableTaskInfo extends TaskInfo { | ||
| /** terminal id. Defined if task is run as a terminal process */ | ||
| readonly terminalId: number, | ||
| } |
There was a problem hiding this comment.
I don't see the reason for AttachableTaskInfo, why wasn't TaskInfo sufficient?
There was a problem hiding this comment.
@vince-fugnitto Actually AttachableTaskInfo is not necessary. I'll redo all the changes made to task-protocol.ts.
| const runningTasks: TaskInfo[] = await this.getRunningTasks(); | ||
| // Get the corresponding task information based on task id if available. | ||
| const taskInfo: TaskInfo | undefined = runningTasks.find((t: TaskInfo) => t.taskId === taskId); | ||
| async attach(taskInfo: AttachableTaskInfo): Promise<void> { |
There was a problem hiding this comment.
This is a breaking change.
If it is necessary it should be included in the CHANGELOG.
…t-lived tasks. Signed-off-by: Cai Xuye <[email protected]>
a1994846931931
force-pushed
the
fix-short-lived-task
branch
from
6691f88 to
6c0258f
Compare
|
@vince-fugnitto Hi Fugnitto, thanks for your advice. The |
| this.willReapProcesses.set(process.id, process); | ||
| setTimeout(() => { | ||
| this.willReapProcesses.delete(process.id); | ||
| }, this.REAP_TIMEOUT); |
There was a problem hiding this comment.
I did exactly the same thing in my local environment to get around this bug.
I am not sure if this is how it should be fixed. I will leave it to @marechal-p and @akosyakov :)
There was a problem hiding this comment.
@elaihau Haha, what a coincidence. The idea is that if one wants to manipulate, for example, write to that process, or resize the terminal process and so on, the process should be alive, and getAlive may be called (a more reliable way is to get the process by either method, and check its killed property right before manipulating it); otherwise, get method can be used and, although the process may be dead, the caller can still get the output stream or other attached information to that process. The getAlive method is just equivalent to the former get method.
There was a problem hiding this comment.
I think the proper solution is to have clear lifecycles and don't rely on timings: #2961 (comment)
|
@a1994846931931 it's been a while since the pull-request has been updated, do you still plan on working on it? |
|
The issue has been resolved thanks to #9409. |
Signed-off-by: Cai Xuye [email protected]
What it does
How to test
echo hello, and check the task terminal widget for its title and output;Demo:
Before
After
Review checklist
Reminder for reviewers