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
"Developer: Reload Window" causes issues for long running tasks #180243
Comments
thanks for looking into this. the reason it's not in a the only tasks that get reconnected to are those with
|
task reconnection works for me |
Ok. So there's a race that happens between deciding to launch the task and actually launching it? In that case, the solution seems kind of misleading. Shouldn't it be something like
why not just do
And why do you need the
But all this is incidental; I was hoping this code could prevent the bug I reported, but clearly it can't. |
Ok... but the problem is that my task is left running across the reload, but it doesn't get reconnected (apparently intentionally), so attempting to start it again succeeds, even though it's supposed to only allow one instance.
I'm not sure what that .mov is supposed to be showing me; I can't see anything relating to tasks being reconnected (but I'm not sure what Im looking for). But anyway, I'm not claiming it's broken; just that it doesn't reconnect my task (or similar ones). And if, as you say, thats intentional, then the bug is elsewhere. But as far as I can tell, either my task shouldn't be left running, or it should be reconnected. |
Another data point. After reloading, the task is clearly running, but |
So when I set this to false, my task is killed on window reload, which does mitigate the problem (in fact, for my use case, I think this does solve the problem - although I don't know yet whether it introduces new ones). But that still leaves us with the issue when |
I won't be looking into this any time soon just fyi. So if it bothers you, feel free to keep digging. Otherwise, just keep the setting disabled. |
I run into this problem all the time too, which is very frustrating. I have to reload often due to SSH reconnection etc, this reattach issue together with ssh agent forwarding on mac os breaking, are really detrimental to the work flow. Using |
Yeah @patroza that is unrelated to this feature as you mentioned. |
When the window is reloaded, and there are long running, non-background tasks the tasks are left running, but aren't added to the `TerminalTaskSystem`'s `_instances` or `_activeTasks` maps after the reload. As a result, various commands, such as `workbench.action.tasks.terminate`, `workbench.action.tasks.restartTask`, `workbench.action.tasks.showTasks` all report that there are no running tasks - even though the tasks are in fact running. In addition, trying to start the tasks starts a new copy - even if the task's `runOptions.instanceLimit` is set to 1 (the default). Note that prior to the reload, trying to start one of the already running tasks would pop up a prompt asking if you want to stop or restart the task. Finally, if you have a task configured as `runOn: folderOpen` that is still running when the window is reloaded, the original task will still be running after the reload, and a new copy will be started in addition. This commit partially fixes these issues. By running `this._reconnectToTerminals()` from `TerminalTaskSystm`'s constructor, it ensures that any future attempt to run one of the already running tasks will simply select its terminal, while also adding the task to `_instances` or `_activeTasks`, so that further attempts to run it will present the prompt as expected. This completely fixes the issue of allowing multiple copies of a task to run when its configured with an `instanceLimit` of 1. It also completely fixes all the issues for any `runOn: folderOpen` tasks. It doesn't address the issue where various commands report that there are no running tasks; but that's no worse than before, and it can now be fixed by simply launching the tasks manually (which doesn't actually start new copies)
@meganrogge as noted in the pull request, this doesn't fix everything, but it makes my workflow (with several auto-run tasks) much cleaner. Those tasks will be properly reconnected on reload, rather than an additional set being kicked off on each reload. For tasks that don't auto-run on reload, the only change is that the first attempt to run a new instance of that task will simply attach to the existing process; and subsequent attempts will fail the same way they do before the reload. Other than that, there should be no change in behavior, so I think this should be pretty safe. I spent some time trying to figure out how to actually reconnect all the running tasks, rather than just marking them for reconnection, but I kept running into issues where abstractTaskService.ts needed to know about the internals of terminalTaskSystem.ts ( I also tried a completely different approach - namely, including non-background tasks in |
@markw65 thanks for looking into this. I commented on your PR |
When the window is reloaded, and there are long running, non-background tasks the tasks are left running, but aren't added to the `TerminalTaskSystem`'s `_instances` or `_activeTasks` maps after the reload. As a result, various commands, such as `workbench.action.tasks.terminate`, `workbench.action.tasks.restartTask`, `workbench.action.tasks.showTasks` all report that there are no running tasks - even though the tasks are in fact running. In addition, trying to start the tasks starts a new copy - even if the task's `runOptions.instanceLimit` is set to 1 (the default). Note that prior to the reload, trying to start one of the already running tasks would pop up a prompt asking if you want to stop or restart the task. Finally, if you have a task configured as `runOn: folderOpen` that is still running when the window is reloaded, the original task will still be running after the reload, and a new copy will be started in addition. This commit partially fixes these issues. By running `this._reconnectToTerminals()` from `TerminalTaskSystm`'s constructor, it ensures that any future attempt to run one of the already running tasks will simply select its terminal, while also adding the task to `_instances` or `_activeTasks`, so that further attempts to run it will present the prompt as expected. This completely fixes the issue of allowing multiple copies of a task to run when its configured with an `instanceLimit` of 1. It also completely fixes all the issues for any `runOn: folderOpen` tasks. It doesn't address the issue where various commands report that there are no running tasks; but that's no worse than before, and it can now be fixed by simply launching the tasks manually (which doesn't actually start new copies)
To verify, run a non background task. Reload the window. Verify that you can run that task again |
Does this issue occur when all extensions are disabled?: Yes
Steps to Reproduce:
Run the following commands
Trust the workspace, then start the script from the "NPM Scripts" panel. The script runs, and the name of the script appears to the right of the output panel, together with a spinner showing that it's running.
Now run the script again. A popup window appears saying that it's already running, and do you want to terminate or restart it (just close it without doing either).
This is all expected behavior.
Now (with the task running) run Cmd-Shift-P "Developer: Reload Window". The window reloads, and the script continues to run (you can tell it's the same script because it's printing its pid, and it doesn't change). The name is still in the right hand pane, but now the spinner has gone, as if vscode no longer knows that it's running.
Now run the script again, and although it's already running, it starts a second copy. This is the bug. It's especially painful for "runOnOpen" tasks, because every time you reload the window (eg because an extension needs updating) they all continue to run AND a new copy of each one starts.
I thought there might be another bug in the recently added
AbstractTaskService._inProgressTasks
(since that was supposed to fix exactly this issue - #159640), and took a look at that. There does appear to be a pretty serious bug in that code:vscode/src/vs/workbench/contrib/tasks/browser/abstractTaskService.ts
Lines 1835 to 1858 in e9c1114
It adds
qualifiedLabel
to the_inProgressTasks
at line 1839, then starts the task at line 1853, and then removesqualifiedLabel
at line 1855 without waiting for the task to finish. So in practice the list is almost always empty, and doesn't actually do anything. I think thedelete
at line 1855 should actually be in a.finally
clause attached to the_handleExecuteResult(...)
on line 1856, so that it doesn't run until the task completes.But fixing that doesn't actually fix this issue, because the set isn't preserved (or reconstructed) across reloads. In addition it breaks the above behavior where trying to run an already running task asks what you want to do with it (because now that inProgressTasks is actually working, it aborts the attempt to re-run the task before it gets to the point where that dialog is presented). It also breaks tasks that are explicitly marked as allowing more than one instance.
So since it isn't currently doing anything, and fixing it breaks things, I would suggest removing all the associated code (@meganrogge)
I'm not sure what the actual fix should be though. There seems to be code to try to reconnect tasks to terminals:
vscode/src/vs/workbench/contrib/tasks/browser/abstractTaskService.ts
Lines 340 to 341 in e9c1114
but it doesn't seem to work (I've stepped through the code with my example, and it goes into
_attemptTaskReconnection
but doesn't actually find any tasks to reconnect). So maybe the bug is there. Or maybe the bug is that the tasks shouldn't be preserved across reload (many are not; a terminal that isn't running anything will be killed by reload, for example).But I've reached the limits of what I can debug, unless anyone has suggestions for where to look.
The text was updated successfully, but these errors were encountered: