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(cmd/main.go): add a channel to proc{} for detecting SIGINT #210

Merged

Conversation

johnrichardrinehart
Copy link

@johnrichardrinehart johnrichardrinehart commented Aug 12, 2021

Fixes #209

Description

This is a solution to fix the usability problem described in #209.

Please provide any suggestions to improve the code quality. Thank you.

Use an atomic int to track the signal code.
Add 128 to the signal to match bash.
Return a pointer to proc, so that the caller is able to load the signal
number.
Copy link
Member

@dnephin dnephin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for raising this issue and opening a PR to fix it!

Your changes were a great start. I had to make a few changes to get it to work:

  • replaced the channel with an in32 that is accessed atomically. We don't need coordination here, so the atomic int seems like it should work well without having to deal with reading from a channel that could be empty
  • changed startGoTest to return a pointer to a proc, so that the caller would be reading the same field as the signal handler function.
  • changed main() to accept an exit code from any error, instead of only from an exec.ExitError.
  • add 128 to the signal number to match bash (https://tldp.org/LDP/abs/html/exitcodes.html)

This seems to work for me, let me know if it is also working for you.

@johnrichardrinehart
Copy link
Author

johnrichardrinehart commented Aug 17, 2021

@dnephin Awesome! Happy you considered my PR. Please give me 24 hours to review.

One question, to start: Did my code fail to compile/execute, for you? I compiled and executed in my fork prior to submitting the PR.

I'm confused why the atomic load-store was necessary, but I trust that it was. Would you please explain this to me? I figured my channel solution would handle synchronization/races.

I just re-read your comment. I think the atomic load-store is a much better semantic than a signal channel. Good job!

I love that you found the bash section on exit codes and applied it correctly. Thanks!

Thanks for taking the time to clean this up. I fully expected some revisions, so I'm happy to see your commits :) .

@dnephin
Copy link
Member

dnephin commented Aug 23, 2021

One question, to start: Did my code fail to compile/execute, for you?

Ya, my comment was misleading I think. I believe it did work, but it felt to me like the atomic was going to be slightly safer and easier to work with.

Thanks again for raising this issue and getting it fixed!

@dnephin dnephin merged commit 37116ff into gotestyourself:main Aug 23, 2021
@johnrichardrinehart johnrichardrinehart deleted the fix/detect-death-by-sigint branch November 13, 2021 14:30
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.

SIGINT is indistinguishable from a failed test
2 participants