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

fix: kill other running tasks on failure

Merged
merged 5 commits into from Mar 16, 2022

Conversation

s-h-a-d-o-w
Copy link
Contributor

@s-h-a-d-o-w s-h-a-d-o-w commented Mar 13, 2022

This kills already running tasks as soon as an error is emitted by another task. Making it possible to interrupt concurrently running tasks on first failure.

Aside from the included unmocked test, you can also try it out by staging a linting error and changing the lint-staged configuration in this repo to .lintstagedrc.mjs with the following content:

export default {
  "*.js": "eslint",
  "*.{js,ts}": () => "npm run test",
}

... and running:
node ./bin/lint-staged.js

I have changed the precedence of the tags, since I think it would be confusing to users to see the signals when we consciously kill tasks. (With the previous order, they would have seen SIGTERM. This way, it is KILLED.)

There are two minor caveats:

  • It doesn't work with shell: true.
  • For this to work with npm scripts (e.g. vanilla tsc, jest and probably anything that deals properly with its children is just fine), we need to kill all children ourselves. Which execa currently doesn't do for a good reason. However, I would argue that while this makes sense for an all purpose tool like execa, given lint-staged's use case, I can't imagine danger from killing child processes within milliseconds.

Fixes #594

@s-h-a-d-o-w s-h-a-d-o-w force-pushed the fix/kill-concurrent-tasks-on-failure branch from 0402ef7 to 66f8c04 Compare Mar 13, 2022
@iiroj
Copy link
Collaborator

iiroj commented Mar 14, 2022

Hello,

thanks for the PR. Currently we use listr2 for handling task lifecycle — I wonder if it has a built-in way to cancel parallel tasks on failure. Also, execa can use an AbortController signal, would it possible to use that somehow?

@iiroj
Copy link
Collaborator

iiroj commented Mar 14, 2022

I believe we are intentionally disabling this error for parallel tasks, and only enabling it for subtasks (that run serially):

lint-staged/lib/runAll.js

Lines 143 to 149 in 531275c

const listrOptions = {
ctx,
exitOnError: false,
nonTTYRenderer: 'verbose',
registerSignalListeners: false,
...getRenderer({ debug, quiet }),
}

@s-h-a-d-o-w
Copy link
Contributor Author

s-h-a-d-o-w commented Mar 14, 2022

I believe we are intentionally disabling this error for parallel tasks

That was the first thing I tried. It just marks the task as interrupted in the terminal but doesn't actually exit.

I wonder if it has a built-in way to cancel parallel tasks on failure

Like you mention - it's just the task life cycle. We manage what those tasks actually do in resolveTaskFn.js with execa.

Also, execa can use an AbortController signal, would it possible to use that somehow?

That would work as well but...

  • It is only supported starting from node 15. They claim that it is available in 14 too but using 14.19.0, I get: ReferenceError: AbortController is not defined
  • I'm not sure whether it is reliable when it comes to killing children. While it does seem to work with both npm and yarn at the moment, given that neither of them used to pass along signals to their children, I don't know why this works all of a sudden and whether it will continue to work in the future.

It is a more elegant solution though, so if you were to argue that I should check for the existence of AbortController and we will gamble on the fact that it will continue to work with children, I'm happy to make those changes.

@iiroj
Copy link
Collaborator

iiroj commented Mar 14, 2022

Thanks for the explanation! Just to make sure, does this kill all tasks or just the subtasks that we currently set to fail on error? We should still sync those settings.

lib/resolveTaskFn.js Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 14, 2022

Codecov Report

Merging #1117 (54b151d) into master (517235d) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #1117   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           25        25           
  Lines          678       691   +13     
  Branches       179       180    +1     
=========================================
+ Hits           678       691   +13     
Impacted Files Coverage Δ
lib/resolveTaskFn.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 517235d...54b151d. Read the comment docs.

lib/resolveTaskFn.js Outdated Show resolved Hide resolved
lib/resolveTaskFn.js Outdated Show resolved Hide resolved
@s-h-a-d-o-w s-h-a-d-o-w requested a review from iiroj Mar 14, 2022
@iiroj
Copy link
Collaborator

iiroj commented Mar 15, 2022

Looks like Node.js 12 Windows tests failed, random?

@s-h-a-d-o-w
Copy link
Contributor Author

s-h-a-d-o-w commented Mar 15, 2022

I happen to be on windows and so tried to reproduce this. I'm seeing more, different errors even on master though.
A colleague who is also on windows said master fails for him too. But I still need to sync with him on details.

@s-h-a-d-o-w
Copy link
Contributor Author

s-h-a-d-o-w commented Mar 15, 2022

That said, I believe this might have to do with the fact that I didn't mock pidtree and maybe it tries to get something like pid 0 and that returns something on nix systems but not on windows (just a theory).
I'll obviously mock that asap, wouldn't want to try killing random things when running tests. 😅

@iiroj
Copy link
Collaborator

iiroj commented Mar 15, 2022

Our Windows tests could always use more love and attention, thanks.

@s-h-a-d-o-w
Copy link
Contributor Author

s-h-a-d-o-w commented Mar 15, 2022

Just ran this with #1118 merged in locally in my Windows 11 environment and at least here, everything passes now. 👍

@iiroj
Copy link
Collaborator

iiroj commented Mar 16, 2022

It's probably not related to this, but I'm getting double-output from terminal:

❯ node bin/lint-staged.js
✔ Preparing lint-staged...
✔ Hiding unstaged changes to partially staged files...
⚠ Running tasks for staged files...
  ❯ .lintstagedrc.mjs — 1 file
    ❯ *.mjs — 1 file
      ✖ prettier --list-different [FAILED]
    ❯ *.{mjs,js,ts} — 1 file
      ✖ npm run test
↓ Skipped because of errors from tasks. [SKIPPED]
↓ Skipped because of errors from tasks. [SKIPPED]
✔ Reverting to original state because of errors...
✔ Cleaning up temporary files...

✖ prettier --list-different:
.lintstagedrc.mjs
✔ Preparing lint-staged...
✔ Hiding unstaged changes to partially staged files...
⚠ Running tasks for staged files...
  ❯ .lintstagedrc.mjs — 1 file
    ❯ *.mjs — 1 file
      ✖ prettier --list-different [FAILED]
    ❯ *.{mjs,js,ts} — 1 file
      ✖ npm run test [KILLED]
↓ Skipped because of errors from tasks. [SKIPPED]
↓ Skipped because of errors from tasks. [SKIPPED]
✔ Reverting to original state because of errors...
✔ Cleaning up temporary files...

iiroj
iiroj approved these changes Mar 16, 2022
Copy link
Collaborator

@iiroj iiroj left a comment

Great work!

lib/resolveTaskFn.js Show resolved Hide resolved
@iiroj iiroj merged commit 34fe319 into okonet:master Mar 16, 2022
19 checks passed
@github-actions
Copy link

github-actions bot commented Mar 16, 2022

🎉 This PR is included in version 12.3.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

@s-h-a-d-o-w
Copy link
Contributor Author

s-h-a-d-o-w commented Mar 16, 2022

It's probably not related to this, but I'm getting double-output from terminal

I noticed that too and double-checked with previous master, where I saw it happen as well (no fancy options - just running default concurrently and waiting for tests to finish running). I figured that you might do this on purpose, so that the final output is always the list of tasks, even when there are potentially long error logs.

Great work!

Thanks! It's been a pleasure working with you on this. 🙂

@iiroj
Copy link
Collaborator

iiroj commented Mar 16, 2022

It might be that we print the task error output too soon, and listr2 does some final update after that, producing the duplicate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants