cmd/vet, os/signal: possible false-positive in sigchanyzer check when os.Signal channel is unused #45043

dmitshur · 2021-03-15T21:39:15Z

What version of Go are you using (`go version`)?

$ gotip version
go version devel +8ed438c07 Mon Mar 15 20:48:37 2021 +0000 darwin/amd64

Does this issue reproduce with the latest release?

No.

What operating system and processor architecture are you using (`go env`)?

go env Output

$ gotip env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/dmitshur/Library/Caches/go-build"
GOENV="/Users/dmitshur/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/dmitshur/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/dmitshur/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/Users/dmitshur/sdk/gotip"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users/dmitshur/sdk/gotip/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="devel +8ed438c07 Mon Mar 15 20:48:37 2021 +0000"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/var/folders/zb/5p8cwfhj29gf_m8vdy8ylmlr00jwcj/T/tmp.YEX3UfOo/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/zb/5p8cwfhj29gf_m8vdy8ylmlr00jwcj/T/go-build3265915026=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I wrote a program that appears to be correct. It uses signal.Notify to catch SIGINT signals to prevent default handling but otherwise completely ignore them:

package main

import (
	"os"
	"os/signal"
	"time"
)

func main() {
	signal.Notify(make(chan os.Signal), os.Interrupt) // ignore SIGINT
	time.Sleep(10 * time.Second)
}

Then ran gotip vet . on it.

What did you expect to see?

No vet report, because as far as I can tell the os.Signal channel doesn't need to be buffered when it's completely unused.

What did you see instead?

$ gotip vet .
# m.test
./main.go:10:2: misuse of unbuffered os.Signal channel as argument to signal.Notify

CC @odeke-em, @ianlancetaylor, @timothy-king.

The text was updated successfully, but these errors were encountered:

timothy-king · 2021-03-15T23:43:02Z

I am not sure I understand why this is safe yet. (So no informed cmd/vet opinion yet.)

zikaeroh · 2021-03-15T23:49:06Z

This came up in CL 217765 for #36976, where I copied a function from cmd/go/internal/base to the dl wrapper to make sure the parent process doesn't also spit out traces. But, cmd/go needs to actually use the signal to make a note that it was interrupted. dl doesn't, so doesn't actually care whether or not it can observe the signal (it's just going to exit once the child is gone anyway).

ianlancetaylor · 2021-03-16T00:00:40Z

Code like signal.Notify(make(chan os.Signal), os.Interrupt) is safe because the runtime always writes to the channel using a non-blocking send. This is in effect a way to ignore the signal. (It would be better to use signal.Ignore, but that is a separate issue.)

It's not safe to write code like

    c := make(chan os.Signal)
    signal.Notify(c, os.Interrupt)
    // use c somewhere

because the fact that the runtime uses a non-blocking send means that if the channel is unbuffered signals can be lost. In the case where the make is invoked directly in the call to signal.Notify, nothing is ever going to read from the channel, so the fact that signals can be lost is unimportant.

odeke-em · 2021-03-16T00:08:29Z

Roger that, thanks everyone for the discussion. I shall send a change tomorrow morning/afternoon, and get this fixed.

zikaeroh · 2021-03-16T00:10:06Z

Thanks for the pointer to signal.Ignore, I didn't remember that there was a distinct function for this. (I don't think I can use it in my CL anyway, given I assume golang.org/dl probably still needs to support Go 1.4. Oh well.)

AGMETEOR · 2021-03-23T16:30:26Z

@odeke-em , I would like to fix this in the sigchanyzer

odeke-em · 2021-03-23T22:14:18Z

All yours, thank you @AGMETEOR and welcome to the Go project, great to have you become a contributor!

ianlancetaylor · 2021-04-19T23:00:26Z

Is the pull request for this going to get in for 1.17? Thanks.

odeke-em · 2021-04-19T23:02:24Z

It is! Just got stalled. @cuonglm, let’s take over, @AG-Meteor we can select another bug later on, thanks for the contributions and offline we can work on something else :-)

…

On Mon, Apr 19, 2021 at 4:00 PM Ian Lance Taylor ***@***.***> wrote: Is the pull request for this going to get in for 1.17? Thanks. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#45043 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABFL3V34M3M74RVMIEMF3IDTJSYZ3ANCNFSM4ZHJ6TCA> .

AGMETEOR · 2021-04-19T23:38:52Z

I am implementing @cuonglm feedback now. If by EOD I haven't finished then he can take over
Update: @odeke-em I have made some changes to the PR. Hopefully @cuonglm has some time to take a look asap.

gopherbot · 2021-04-22T14:04:26Z

Change https://golang.org/cl/311729 mentions this issue: passes/sigchanyzer: allow valid inlined unbuffered signal channel

Permit signal.Notify(make(chan os.Signal)) which is valid code, given that the channel isn't read elsewhere, the fact that signals can be lost is unimportant. Updates golang/go#45043 Change-Id: Ie984dfeedbb4e1e33a29391c3abb1fc83299fed3 Reviewed-on: https://go-review.googlesource.com/c/tools/+/311729 Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com> Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com> Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com> Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com> gopls-CI: kokoro <noreply+kokoro@google.com> TryBot-Result: Go Bot <gobot@golang.org>

gopherbot · 2021-04-23T02:34:20Z

Change https://golang.org/cl/312631 mentions this issue: cmd/vendor: get golang.org/x/tools@f946a157eef

wnz27 · 2021-11-25T11:44:46Z

I can't understant upgrade golanglint but it still: sigchanyzer: misuse of unbuffered os.Signal channel as argument to signal.Notify

ianlancetaylor · 2021-11-26T20:45:34Z

@wnz27 This issue is closed and we believe that it is fixed. If you have a case where the warning is being issued inappropriately, please open a new issue with instructions for how to reproduce the problem. If you are asking a question, please see https://golang.org/wiki/Questions. Thanks.

wnz27 · 2021-11-28T07:21:07Z

@wnz27 This issue is closed and we believe that it is fixed. If you have a case where the warning is being issued inappropriately, please open a new issue with instructions for how to reproduce the problem. If you are asking a question, please see https://golang.org/wiki/Questions. Thanks.

thanks， i feel sorry for that

This looks like a false positive in go vet. See golang/go#45043 However, it should be fine to just buffer the channel to silence go vet, as was discussed in the issue. Ideally we could silence the error, but there is no way to ignore a line in a file for go vet: golang/go#17058

dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 15, 2021

dmitshur added this to the Backlog milestone Mar 15, 2021

odeke-em self-assigned this Mar 16, 2021

odeke-em modified the milestones: Backlog, Go1.17 Mar 16, 2021

odeke-em added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Mar 16, 2021

AGMETEOR mentioned this issue Mar 23, 2021

Fix false-positive in sigchanyzer check when os.Signal channel is unused orijtech/sigchanyzer#8

Merged

zpavlinovic mentioned this issue Apr 16, 2021

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

Closed

odeke-em closed this as completed in orijtech/sigchanyzer#8 Apr 20, 2021

odeke-em reopened this Apr 20, 2021

gopherbot closed this as completed in 5963f0a Apr 23, 2021

chappjc mentioned this issue May 31, 2021

begin 0.3 decred/dcrdex#1101

Merged

dtomcej mentioned this issue Jun 15, 2021

Create buffered signals channel traefik/traefik#8190

Merged

leighmcculloch mentioned this issue Aug 17, 2021

all: update supported go versions to 1.17 and 1.16 stellar/go#3827

Merged

hickeyma mentioned this issue Nov 16, 2021

fix(install/upgrade): Use buffered channel for signal notification to avoid signal loss helm/helm#10347

Merged

3 tasks

rsc unassigned odeke-em Jun 23, 2022

golang locked and limited conversation to collaborators Jun 23, 2023

gopherbot added the FrozenDueToAge label Jun 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/vet, os/signal: possible false-positive in sigchanyzer check when os.Signal channel is unused #45043

cmd/vet, os/signal: possible false-positive in sigchanyzer check when os.Signal channel is unused #45043

dmitshur commented Mar 15, 2021

timothy-king commented Mar 15, 2021

zikaeroh commented Mar 15, 2021

ianlancetaylor commented Mar 16, 2021

odeke-em commented Mar 16, 2021

zikaeroh commented Mar 16, 2021

AGMETEOR commented Mar 23, 2021

odeke-em commented Mar 23, 2021

ianlancetaylor commented Apr 19, 2021

odeke-em commented Apr 19, 2021 via email

AGMETEOR commented Apr 19, 2021 •

edited

gopherbot commented Apr 22, 2021

gopherbot commented Apr 23, 2021

wnz27 commented Nov 25, 2021

ianlancetaylor commented Nov 26, 2021

wnz27 commented Nov 28, 2021

cmd/vet, os/signal: possible false-positive in sigchanyzer check when os.Signal channel is unused #45043

cmd/vet, os/signal: possible false-positive in sigchanyzer check when os.Signal channel is unused #45043

Comments

dmitshur commented Mar 15, 2021

What version of Go are you using (go version)?

Does this issue reproduce with the latest release?

What operating system and processor architecture are you using (go env)?

What did you do?

What did you expect to see?

What did you see instead?

timothy-king commented Mar 15, 2021

zikaeroh commented Mar 15, 2021

ianlancetaylor commented Mar 16, 2021

odeke-em commented Mar 16, 2021

zikaeroh commented Mar 16, 2021

AGMETEOR commented Mar 23, 2021

odeke-em commented Mar 23, 2021

ianlancetaylor commented Apr 19, 2021

odeke-em commented Apr 19, 2021 via email

AGMETEOR commented Apr 19, 2021 • edited

gopherbot commented Apr 22, 2021

gopherbot commented Apr 23, 2021

wnz27 commented Nov 25, 2021

ianlancetaylor commented Nov 26, 2021

wnz27 commented Nov 28, 2021

What version of Go are you using (`go version`)?

What operating system and processor architecture are you using (`go env`)?

AGMETEOR commented Apr 19, 2021 •

edited