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

os: update signal mutex critical region #17266

Closed
ggaaooppeenngg opened this issue Sep 28, 2016 · 2 comments
Closed

os: update signal mutex critical region #17266

ggaaooppeenngg opened this issue Sep 28, 2016 · 2 comments

Comments

@ggaaooppeenngg
Copy link

@ggaaooppeenngg ggaaooppeenngg commented Sep 28, 2016

Please answer these questions before submitting your issue. Thanks!

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

go version go1.7 darwin/amd64

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/mike/workplace/GOPATH"
GORACE=""
GOROOT="/Users/mike/workplace/go"
GOTOOLDIR="/Users/mike/workplace/go/pkg/tool/darwin_amd64"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/z4/vplvdvkx6fsfv7z6w8pzfqnr0000gn/T/go-build180986648=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"

What did you do?

If possible, provide a recipe for reproducing the error.
A complete runnable program is good.
A link on play.golang.org is best.

none

What did you expect to see?

signal mutex locks Kill syscall only.

What did you see instead?

signal mutex lock process done flag also, but it is atomic, can be checked first.

I just find , in os package func (p *Process) signal(sig Signal) error, p.done() is in the p.sigMu protection, but in fact p.done() is atomic, place it before p.sigMu lock, signal call to done process can return more quickly.

@bradfitz bradfitz changed the title update signal mutex critical region os: update signal mutex critical region Sep 28, 2016
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Sep 28, 2016

Does this matter for your application?

The benefits of changing it seem incredibly small to me. I'd be surprised if you can measure a difference.

/cc @ianlancetaylor

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Sep 28, 2016

Russ did a code review on email and said:

Also, the code is correct as written and incorrect as patched.

Locks protect invariants in your data structure, not just individual words of memory. It's true that the memory race detector won't complain if you move those lines, but there would be a semantic race introduced between signal and wait.

Closing as not a bug.

@bradfitz bradfitz closed this Sep 28, 2016
@golang golang locked and limited conversation to collaborators Sep 28, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.