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

Support reconnect in tasks #117408

Closed
Tyriar opened this issue Feb 23, 2021 · 10 comments · Fixed by #155120
Closed

Support reconnect in tasks #117408

Tyriar opened this issue Feb 23, 2021 · 10 comments · Fixed by #155120
Assignees
Labels
feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders on-testplan tasks Task system issues
Milestone

Comments

@Tyriar
Copy link
Member

Tyriar commented Feb 23, 2021

Terminals now reconnect when reloading local or remote windows. If a task is running at the time this will still reconnect the terminal but not restore the task, for example the VS Code core watch task results in this:

image

cc @meganrogge

@alexr00
Copy link
Member

alexr00 commented Feb 23, 2021

Yes, this would be great. I thought @alexdima had done some work to make sure that task terminals were not restored for now though? Since we cannot restore the task now, we decided this was the best behavior for now.

@alexr00 alexr00 added feature-request Request for new features or functionality tasks Task system issues labels Feb 23, 2021
@alexr00 alexr00 added this to the Backlog milestone Feb 23, 2021
@alexdima
Copy link
Member

@alexr00 I think what we did was for restored remote terminals, while this issue might be about restoring terminals with local windows.

@Tyriar
Copy link
Member Author

Tyriar commented Feb 23, 2021

Looks like the disabling in remote is here:

!this._shellLaunchConfig.isFeatureTerminal && this._configHelper.config.enablePersistentSessions,

We don't have this for local atm, should we add it? Isn't it good that the task terminals still get restored, despite not being linked to the task system?

@alexr00
Copy link
Member

alexr00 commented Feb 24, 2021

I think having "orphaned" task terminals is not a good behavior, and it would be better if they simply didn't restore until we have a good story. For example, if a user has a task that runs automatically when a folder is opened and their terminal restored then they could get weird behavior because now they have the old orphaned task running and the new task running.

@Tyriar
Copy link
Member Author

Tyriar commented Feb 24, 2021

👍 disabling in #117578

@Tyriar
Copy link
Member Author

Tyriar commented Apr 25, 2021

This would be really nice to have. Right now all I need to do to get back up and running after reloading the window is ctrl+shift+b. This isn't that big of a deal for the VS Code repo or other repos I work in as they're fairly small, but larger projects that don't have the daemon worker system it will mean no more rebuilding after reloads.

@bpasero
Copy link
Member

bpasero commented May 17, 2021

This would be really nice to have. Right now all I need to do to get back up and running after reloading the window is ctrl+shift+b. This isn't that big of a deal for the VS Code repo or other repos I work in as they're fairly small, but larger projects that don't have the daemon worker system it will mean no more rebuilding after reloads.

Same here, would be great to have for a nice reload/restart experience. I. keep many local checkouts of VSCode to try out different things and switch branches and I often forget in which workspace I have background compile tasks running.

@Tyriar
Copy link
Member Author

Tyriar commented Jun 13, 2022

We want to explore this to try to gauge how much effort it will take to do, reconnection reliability is the priority for June #151936

@meganrogge
Copy link
Contributor

explored this on origin/merogge/task-reconnection. some questions:

  1. TerminalTaskSystem is lazily created ATM, but to reconnect to tasks, we'll need it to be created when the window is loaded so it re-runs tasks when TerminalService has reconnected to terminals and fires _onDidRequestTasksReconnection
  2. AbstractTaskSystem is aware / the source of the ITaskResolver that is needed to run each task. AbstractTaskSystem is unaware of ITerminalService currently, so doesn't seem like the right place for reconnection. Does this mean we need to store the resolver for the last task run in each terminal in TerminalTaskSystem? Had some trouble doing this via lastTask.resolver.

@meganrogge meganrogge modified the milestones: June 2022, July 2022 Jun 17, 2022
@alexr00
Copy link
Member

alexr00 commented Jun 23, 2022

@meganrogge I'm looking at your work so far now, but I've been thinking about your points/questions. Did you consider having AbstractTaskService save which tasks are running, then upon reload "restart" (in quotes because they never actually stopped) them by calling into the TerminalTaskSystem? The TerminalTaskSystem then would check if there are already terminals running that "match" the task based on some property that is set on the terminal? That would let the task service continue to be the orchestrator of tasks instead of having the terminal task system taking some of that responsibility.

For saving the tasks, I think you can use the tasks recently used key.

@alexr00 alexr00 removed their assignment Jun 30, 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 Jul 14, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Aug 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders on-testplan tasks Task system issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants