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

[perillo]

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]

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]

@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]

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.

[timothy-king]

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

[perillo]

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