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

dl/internal/version: misuse of unbuffered os.Signal channel as argument to signal.Notify #45604

Closed
perillo opened this issue Apr 16, 2021 · 5 comments
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@perillo
Copy link
Contributor

perillo commented Apr 16, 2021

Running gotip vet on internal/version reports: "misuse of unbuffered os.Signal channel as argument to signal.Notify".

golang/dl@5564b1e for #36976 used an unbuffered channel. Was there a reason?

@zpavlinovic
Copy link
Contributor

This discussion suggests this is a simple and safe fix since the signal is simply interrupted and ignored. (In fact, #45043 argues this is a FP of go vet that should be fixed.) signal.Ignore should be used in principle, but legacy aspects of golang.org/dl might cause issues with that approach. The whole discussion can be found in the unresolved comment of patchset 3.

@perillo
Copy link
Contributor Author

perillo commented Apr 17, 2021

@zpavlinovic thanks, that was an interesting discussion.

However making the channel buffered with
signal.Notify(make(chan os.Signal, 1), signalsToIgnore...)
seems to work the same way as with the original unbuffered channel, and it does not trigger the go vet report.

That was the reason why I asked if there was a reason for the channel to be unbuffered.

@zpavlinovic
Copy link
Contributor

The best explanation for this that I was able to find lies in the following comment by @dmitshur:

I'm realizing now that since the goal of this version of the code is to disable default signal handling, but otherwise completely
ignore the signal, there's no reason for the channel to be buffered.

My understanding is that, yes, you can make the channel buffered and the vet would not report anything. But in principle there should be no need for that and vet should not complain here, i.e, it is a false positive, hence #45043. So rather than slightly changing the code to make vet not complain, change the vet so it does not report this as it shouldn't.

Hopefully @dmitshur can provide more info if needed.

@zpavlinovic zpavlinovic added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 19, 2021
@zpavlinovic zpavlinovic added this to the Unplanned milestone Apr 19, 2021
@zpavlinovic zpavlinovic added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Apr 19, 2021
@timothy-king
Copy link
Contributor

@perillo #45043 is now closed. Does this still reproduce after https://go-review.googlesource.com/c/go/+/312631/ ?

@perillo
Copy link
Contributor Author

perillo commented Apr 27, 2021

@timothy-king, this no longer reproduces. Thanks.

@perillo perillo closed this as completed Apr 27, 2021
@golang golang locked and limited conversation to collaborators Apr 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants