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.

Sign up for GitHub

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

Jump to bottom

There is a race in task startup in terminalTaskService.ts #180541

Closed
markw65 opened this issue Apr 21, 2023 · 1 comment · Fixed by #180546
Closed

There is a race in task startup in terminalTaskService.ts #180541

markw65 opened this issue Apr 21, 2023 · 1 comment · Fixed by #180546
Assignees
@meganrogge
Labels
bug Issue identified by VS Code Team member as probable bug insiders-released Patch has been released in VS Code Insiders tasks Task system issues verified Verification succeeded
Milestone
May 2023

Comments

@markw65
Copy link
Contributor

markw65 commented Apr 21, 2023

Type: Bug

The race was reported in #159640 and partially fixed by #165560.

The fix doesn't fully cover the issue though. The problem is that the task identifier is added to _inProgressTasks just before terminalTaskSystem._executeTask() is called, and removed just after. But _executeTask will return (via "await") before updating _activeTasks. So _activeTasks won't be updated until the next tick (at the earliest, and possibly much much later).

This is a problem because although terminalTaskSystem updates _instances immediately (so it knows how many instances of a task are running), it ignores that unless there's an entry in _activeTasks.

As a result, a task can be re-submitted after its name has been removed from _inProgressTasks, but before the first task has been added to _activeTasks, bypass the instanceLimit check, and start a second, invalid instance. And this is the root cause of #159640.

Typically the race can be hard to hit - especially after #165560 - but if the task has a "dependsOn" that runs for more than a few milliseconds, it's easy to repro. Use these commands to create and open a new project with a suitable task.

mkdir -p test/.vscode
cd test
cat <<"EOF" > .vscode/tasks.json
{
  "version": "2.0.0",
  "tasks": [
    {
      "label": "sleep",
      "type": "shell",
      "command": "sleep 2"
    },
    {
      "label": "build",
      "type": "shell",
      "command": "echo 'Running build' && sleep 5 && echo 'Finished build'",
      "dependsOn": ["sleep"],
      "group": {
        "kind": "build",
        "isDefault": true
      }
    }
  ]
}
EOF
code .

Trust the workspace, then hit Cmd-Shift-B three times in less than 2 seconds (but not too fast, or _inProgressTasks will stop the second and third attempts). You will get three instances of the task running.

VS Code version: Code 1.77.3 (704ed70, 2023-04-12T09:19:37.325Z)
OS version: Darwin x64 22.4.0
Modes:
Sandboxed: No

System Info
Item Value
CPUs Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz (16 x 2300)
GPU Status 2d_canvas: enabled
canvas_oop_rasterization: disabled_off
direct_rendering_display_compositor: disabled_off_ok
gpu_compositing: enabled
metal: disabled_off
multiple_raster_threads: enabled_on
opengl: enabled_on
rasterization: enabled
raw_draw: disabled_off_ok
skia_renderer: enabled_on
video_decode: enabled
video_encode: enabled
vulkan: disabled_off
webgl: enabled
webgl2: enabled
webgpu: disabled_off
Load (avg) 4, 4, 4
Memory (System) 32.00GB (0.66GB free)
Process Argv --crash-reporter-id e8eaf96d-f4d4-4874-a9d9-08e2c6d99409
Screen Reader no
VM 0%
Extensions (22)
Extension Author (truncated) Version
gltf-vscode ces 2.3.16
vscode-eslint dba 2.4.0
githistory don 0.6.20
gitlens eam 13.5.0
prettier-vscode esb 9.12.0
php-intellisense fel 2.3.14
monkey-c Gar 1.0.8
jq-syntax-highlighting jq- 0.0.2
prettier-extension-monkeyc mar 2.0.61
cpptools ms- 1.14.5
js-debug-nightly ms- 2023.4.1317
peggy-language Peg 2.4.0
java red 1.17.0
vscode-xml red 0.25.0
shader sle 1.1.5
rewrap stk 1.16.3
intellicode-api-usage-examples Vis 0.2.7
vscodeintellicode Vis 1.2.30
vscode-java-debug vsc 0.49.1
vscode-java-pack vsc 0.25.10
vscode-java-test vsc 0.38.2
vscode-maven vsc 0.41.0
A/B Experiments 
vsliv368cf:30146710
vsreu685:30147344
python383:30185418
vspor879:30202332
vspor708:30202333
vspor363:30204092
vslsvsres303:30308271
vserr242:30382549
pythontb:30283811
vsjup518:30340749
pythonptprofiler:30281270
vshan820:30294714
vstes263:30335439
vscoreces:30445986
pythondataviewer:30285071
vscod805cf:30301675
binariesv615:30325510
bridge0708:30335490
bridge0723:30353136
cmake_vspar411:30581797
vsaa593:30376534
pythonvs932:30410667
cppdebug:30492333
vsclangdf:30486550
c4g48928:30535728
dsvsc012cf:30540253
pynewext54:30695312
azure-dev_surveyone:30548225
nodejswelcome1:30587005
2e4cg342:30602488
pyind779:30671433
f6dab269:30613381
pythonsymbol12:30671437
vsccsb:30705552
functionswalk:30687959
pythonms35:30701012
pythonfmttextcf:30716743
markw65 pushed a commit to markw65/vscode that referenced this issue Apr 21, 2023
Fix a task startup race
b987e96 
Fixes microsoft#180541

The fix is to enforce `instanceLimit` even when there is no entry in
`_activeTasks`. Once that's done, microsoft#165560 is no longer needed - and is in fact
harmful, since it can incorrectly prevent re-launching a task whose
`instanceLimit` is greater than one.
@markw65 markw65 mentioned this issue Apr 21, 2023
Merged
@meganrogge meganrogge added tasks Task system issues bug Issue identified by VS Code Team member as probable bug labels Apr 21, 2023
@meganrogge meganrogge added this to the May 2023 milestone Apr 21, 2023
@markw65 markw65 mentioned this issue Apr 21, 2023
Closed
@meganrogge meganrogge mentioned this issue May 1, 2023
Closed
@markw65 markw65 mentioned this issue May 4, 2023
Merged
markw65 pushed a commit to markw65/vscode that referenced this issue May 4, 2023
Fix a task startup race
acbd63f 
Fixes microsoft#180541

The fix is to enforce `instanceLimit` even when there is no entry in
`_activeTasks`. Once that's done, microsoft#165560 is no longer needed - and is in fact
harmful, since it can incorrectly prevent re-launching a task whose
`instanceLimit` is greater than one.
@markw65
Copy link
Contributor Author

markw65 commented May 4, 2023

Since this task was filed, task startup has been changed, and a new bug introduced - demonstrated by this tasks.json:

{
  "version": "2.0.0",
  "tasks": [
    {
      "type": "shell",
      "command": "echo dependent",
      "label": "dependent",
      "problemMatcher": []
    },
    {
      "type": "shell",
      "command": "echo Running top level task...;sleep 5;echo done",
      "label": "toplevel",
      "dependsOn": ["dependent"],
      "group": {
        "kind": "build",
        "isDefault": true
      },
      "problemMatcher": []
    }
  ]
}

Hit Cmd-Shift-B a few times; only one task should be launched, and a popup should appear saying that its already running (and offering to kill or restart it) for each subsequent run.

Without this commit, you'll get one task for each Cmd-Shift-B - because instance count checking is completely disabled for tasks with dependencies.

@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 May 4, 2023
@rzhao271 rzhao271 added the verified Verification succeeded label May 31, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Jun 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@meganrogge meganrogge

Labels
bug Issue identified by VS Code Team member as probable bug insiders-released Patch has been released in VS Code Insiders tasks Task system issues verified Verification succeeded
Projects
None yet
Milestone
May 2023
Development

Successfully merging a pull request may close this issue.

Fix a task startup race markw65/vscode
4 participants
@markw65 @rzhao271 @meganrogge @VSCodeTriageBot