Fix signal handling for subprocess #479
Conversation
See go-task/task/go-task#458 Helper (sleepit) and test code based on https://github.com/marco-m/timeit
…c/sh We used to pass to mvdan.cc/sh/interp.Runner a context that was cancelled on reception of a OS signal. This caused the Runner to terminate the subprocess abruptly. The correct behavior instead is for us to completely ignore the signal and let the subprocess deal with it. If the subprocess doesn't handle the signal, it will be terminated. If the subprocess does handle the signal, it knows better than us wether it wants to cleanup and terminate or do something different. So now we pass an empty context just to make the API of interp.Runner happy Fixes go-task/task/go-task#458
|
Ah, the failing tests on CI are due to them not finding the task executable? I will look at them later, the instructions in the PR description do work locally. |
|
Hello @andreynering, any opinions ? :-) |
|
Updated this PR with current master. |
On Github actions, the destination path of "go install" ($GOPATH/bin) is not in $PATH, thus the error. For the life of me I could not understand how to change the $PATH environment variable in an Actions workflow, so I encode the full path of the just-built task executable in the tests, which probably was the right thing to do since the beginning.
|
@andreynering is there a reason why this PR is not being merged? Thanks |
|
I cannot speak for Andrey, but this PR works fine for Unix systems, while I don't know what to do for Windows. Thus merging this PR might make the situation on Windows worse. It is a tricky situation and requires somebody to take the time to test /think on Windows... |
|
I can confirm, this is also what mvdan does in https://github.com/mvdan/sh/blob/master/cmd/gosh/main.go#L63-L64 I had to double check my understanding of subprocess signal handling and confirm that indeed, mvdan is spawning subprocesses, not threads. I think this is a valuable bugfix. |
|
Updated this PR with current master + additional commit removing redundant newline linter error |
|
Running into a similar broken state case using Pulumi and go-task when |
There was a problem hiding this comment.
I think this is well written, with good tests, lots of comments. Given all tests have passed, and the amount of information provided in this PR, associated issue, in the comments and the tests, I think that even if we run into an issue later, it will be easy to follow along and debug.
|
Master build failed immediately after merging. It's possible this introduced flaky tests. |
|
Sorry for not looking into it sooner guys. Since this is a complex problem I wanted to do some tests to try to understand the problem and fix, but I suppose we can trust the intensive tests @marco-m-pix4d did on this 😉. Saw that @ghostsquad opened #728. I ask the people interested to test the fix in master and comment about any issues there on #728. @ghostsquad Small ask: add an entry to CHANGELOG.md whenever a relevant enough feature or fix is added to master (title should be "Unreleased" for now, like this), linking related issue and PRs. I always copy those to the release when the time comes. |
|
I figured that changelog should be updated only during a release. |
|
@ghostsquad Adding there sooner is useful so we don't forgot to add on the release. Also, people interested in the development can see the unreleased changes. Other projects do that, like Ruby on Rails. |
|
Automated generation of the CHANGELOG.md and Release Notes attached to the Github Release via something like https://github.com/git-chglog/git-chglog can handle the "so we don't forget to add". Here's a small (incomplete) snippet of how this can be made possible.
publish:
cmds:
- task: github:env:export
vars:
KEY: GIT_TAG
VALUE: '{{.GIT_TAG}}'
- <something here>
github:env:export:
preconditions:
- '[ "${CI}" == "true" ]'
vars:
KEY: '{{ .KEY | default "TEST_KEY" }}'
VALUE: '{{ .VALUE | default "TEST_VALUE" }}'
cmds:
- echo '{{.KEY}}={{.VALUE}}' >> "${GITHUB_ENV}"
- echo 'This is accessible using {{ printf `${{ env.%s }}` .KEY }}' 1>&2
changelog:
cmds:
- cmd: git-chglog {{.ARGS | default .CLI_ARGS}}
- name: Generate Changelog
run: task changelog -- ${{ env.GIT_TAG }} > ./tmp/CHANGELOG.md
# TODO: inline this into task...
- name: Create a GitHub release
uses: softprops/action-gh-release@v1
with:
token: ${{ secrets.APP_GITHUB_TOKEN }}
body_path: ./tmp/CHANGELOG.md
tag_name: ${{ env.GIT_TAG }} |
|
Oh, though we are already using goreleaser, so this should be taken care of. |
|
Let's move this kind of discussion to the Discord server form now on to void polluting these issues, but... The fact is that I want and like to handwrite the changelog entries / release notes because those are meant to be the more concise and legible as possible by users, to understand what changed recently. I really value when other projects that I use do the same as well. Automation uses commit messages, which are often polluted, unreadable, lacks links to documentation when useful, etc. |
hello @andreynering, the fix is quite simple :-D, what is complicated and worthwhile to spend some time to understand is the first commit a496a75, that introduces extensive tests to reproduce the behavior.
I realized that to fix this problem, it is not possible to tweak the timeout of mvdan.cc/sh, we must be more drastic and don't pass to it any cancellable context. The problem that I observed is the following (reproduced in the tests):
I suggest to checkout a496a75, run
and get acquainted with the (expected) failures.
Then, checkout the tip of the branch and run the same tests, to validate that everything is fixed.
There are some things that are still missing: testing what happens on Windows and deciding how far propagate in the Task functions the fact that the context should not even passed. I mean:
func RunCommand(ctx context.Context, opts *RunCommandOptions)is now lying, since the context will not be passed to anything.As a reference, here are the failing tests on Linux: