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

doc: document that unsynchronized (channel send + channel close) is a data race #27769

Open
deckarep opened this issue Sep 20, 2018 · 10 comments

Comments

@deckarep
Copy link

commented Sep 20, 2018

Now that I'm aware of this behavior i'm only proposing that this be documented in Go's documentation. My rationale is that since channels are built to be threadsafe, I would have assumed all operations against them are threadsafe. ie, sending, receiving, closing.

What version of Go are you using (go version)? go version go1.11 darwin/amd64

Does this issue reproduce with the latest release? Yes

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

GOHOSTARCH="amd64"
GOHOSTOS="darwin"

What did you do?

First off, my code was flawed but it exposed a data-race that I thought would not be possible. My assumption was that all operations on channels are thread-safe by design..at the worst I would have expected the code to panic.

  • Sending on a channel (threadsafe) Well-understood
  • Receiving on a channel (threadsafe) Well-understood
  • Sending on a channel already closed (panics) Well-understood
  • Closing a channel (not threadsafe) Huh?

https://stackoverflow.com/questions/47714470/go-race-condition-when-closing-a-go-channel

What did you expect to see?

I expected a panic to occur but instead this data-race shows up.

What did you see instead?

WARNING: DATA RACE
Read at 0x00c0000b4000 by goroutine 10:
  runtime.chansend()
      /usr/local/Cellar/go/1.11/libexec/src/runtime/chan.go:140 +0x0
  main.tx()
      /Users/deckarep/Desktop/channel_close_race/main.go:15 +0x66

Previous write at 0x00c0000b4000 by main goroutine:
  runtime.closechan()
      /usr/local/Cellar/go/1.11/libexec/src/runtime/chan.go:327 +0x0
  main.main()
      /Users/deckarep/Desktop/channel_close_race/main.go:38 +0x11f
@randall77

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2018

It's reported as a data race, but in reality it's a "your send and close are unordered" error. What it means is that although in this execution the send came before the close, there's no observable ordering enforcing that - the next time you run, it only takes different choices by the scheduler to make the send come after the close and cause a panic.

@deckarep

This comment has been minimized.

Copy link
Author

commented Sep 21, 2018

@randall77 - thanks for the explanation. I think that is an understandable reasoning on why the race detector trips. I'm just questioning if this behavior should be documented...if so where would such docs belong?

@randall77

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2018

@deckarep: I don't really know where such documentation should live. We have docs about how to use the race detector, but I don't see any place to describe the kind of races it finds.

@agnivade

This comment has been minimized.

Copy link
Member

commented Sep 21, 2018

@dvyukov

This comment has been minimized.

Copy link
Member

commented Sep 21, 2018

What @randall77 said.
It's not a /data race/ per se, but it's a bug worth detecting. It's both hard to discriminate this in race detector, and more of a one-off corner case, so race detector just reports this as it reports all other data races.

Re documenting, I don't think there is a single doc that all users of race detector has read before using it. Thus I don't think this can be solved with documentation. But we could mention this in https://golang.org/doc/articles/race_detector.html#Typical_Data_Races

@agnivade agnivade added this to the Unplanned milestone Sep 21, 2018

@deckarep

This comment has been minimized.

Copy link
Author

commented Sep 22, 2018

I’m happy to take a crack and write something up to start off the docs. Then I can consult the Go team to help with wording and accuracy.

@Loveuse

This comment has been minimized.

Copy link

commented Sep 29, 2018

I think https://golang.org/ref/mem under the Channel communication section might also be a good place to mention it since the send on the channel may happen before its closure.

@as

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2019

It would be nice if the documentation also mentioned that this race is a symptom of ineffective producer/consumer designation, not just ordering quirks in the program.

@andybons andybons changed the title Document that unsynchronized (channel send + channel close) is a data race. doc: document that unsynchronized (channel send + channel close) is a data race May 14, 2019

@andybons

This comment has been minimized.

Copy link
Member

commented May 14, 2019

@deckarep are you still interested in doing this?

@tpaschalis

This comment has been minimized.

Copy link

commented Sep 18, 2019

Hope it's okay to bring this up nearly a year after.

I'm looking to contribute for the first time, and documentation seems a safe and easy place to start. The following code still produces a data race.

func TestRace(t *testing.T) {
    var c = make(chan byte, 20)
    go func() {
        defer func() {
            if r := recover(); r == nil {
                t.Error("expected panic error")
            }
        }()
        for i := 0; i < 25; i++ {
            c <- byte(0)
        }
        t.Error("expected a panic")
    }()
    close(c)
}

I could draft a paragraph to document this on https://golang.org/doc/articles/race_detector.html#Typical_Data_Races and get some feedback. Does it sound okay to you?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.