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

feat: Kill child processes when task shell receives ctrl-c #126

Conversation

mo
Copy link

@mo mo commented Nov 13, 2024

When running docker compose from "deno task" you get bad CTRL-C handling compared to "npm run". This problem can be reproduced by running "npm run docker-compose" and "deno task docker-compose" in this minimal testcase repo:
https://github.com/mo/deno-tasks-signal-issue-repro

There is some more info in this issue:
#33


The main purpose of the PR is to make sure all of these behave well with ctrl-c:
{
  "tasks": {
    "docker-compose": "./docker-compose.sh",
    "docker-compose-no-sh-file": "docker compose -f compose.yaml up",
    "sleep": "sleep 999",
    "simple-builtin": "echo hello",
    "simple-bin": "date",
    "cmd": "cat /dev/urandom | head -c 1000000 | sha256sum",
    "hashpipe-slow": "$(which cat) /dev/urandom | $(which head) -c 1000000000 | sha256sum",
    "hashpipe-slower": "$(which cat) /dev/urandom | $(which head) -c 10000000000 | sha256sum",
    "forever-pipe": "yes | grep -v mystery | grep -v magic | sort | uniq -c",
  }
}

When this PR is applied, "deno task docker-compose" works similar to "npm run", however the code is the PR needs some cleanup.

I tried to clean it up by not spawning a tokio task however that version doesn't solve the main docker compose use case and unfortunately I don't understand why that is. This version also prints some debug info with a new empty lines just to make sure the debug print outs doesn't get overwritten by the docker compose "progress" printouts:
mo@7514eba

@dsherret already provided some feedback on needed cleanup of this PR in an earlier PR which was incorrectly pushed with the PR-commit directly on "main".. so the current PR just aims to re-create the PR so that maintainer edits are possible. The old PR was:
#125

@rf-molsson
Copy link

@dsherret if you have some other fix in mind, feel free to discard this PR and merge your own fix instead... I mostly want the issue fixed... and I'd be happy to test our your PR if you create an alternative one, just ping me

@bartlomieju bartlomieju requested a review from dsherret November 14, 2024 13:31
@dsherret
Copy link
Member

Closing in favour of #131

@dsherret dsherret closed this Nov 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants