Actually, in thinking about this some more, the atomics are completely unnecessary. Your solution being the best, but even without it, the channels create a happens before scenario - between the write of closed and “the channel close” and the read of closed. That is, for a channel to be reported as closed all other writes prior must be visible.
> On Sep 2, 2019, at 6:52 PM, Robert Engels <reng...@ix.netcom.com> wrote: > > As far as I know you are attributing properties to the memory model that > don’t yet exist - they’re working on it. Even the “happens before” in > relation to atomics is not yet defined. The position from the Go “team” so > far has been, whatever the behavior you see, that’s the spec (in terms of > memory model) > > Still, I would hope the memory model would cover this case as it would > (probably) avoid unnecessary atomic writes. In this case if there were atomic > Boolean or bytes it wouldn’t be subject to tearing, maybe int32 in most > platforms. > > Still, like I said, if you want add the atomic writes, go ahead. This is > definitely safer. But I think when the full memory model spec arrives it > won’t be necessary. > >> On Sep 2, 2019, at 6:02 PM, roger peppe <rogpe...@gmail.com> wrote: >> >> >> >>> On Mon, 2 Sep 2019, 21:32 robert engels, <reng...@ix.netcom.com> wrote: >>> (you’re comments were quoted, but I think I am replying correctly). >>> >>> The memory barriers provided by the Write Lock, and release will force the >>> flush to memory of all mutations before the flush (the closed mutation) - >>> meaning the atomic read will read the correct value. >> >> >> In this case, I'm much more interested in writing Go code that's correct >> according to the spec than Go code that happens to work on some current >> architecture. Let's try to make code that's "obviously correct" rather than >> "not obviously incorrect". The memory model has no mention of "memory >> barriers" or "flushing". >> >>> >>> There is a chance that a fully specified memory model could make that not >>> be possible, but in the current de-facto memory model it is going to be the >>> case on any CPU architecture I’m aware of. Some transactional memory >>> systems might be able to avoid the flush on the ‘closed’, but unlikely. >>> >>> I would welcome you writing a test case to demonstrate this not being the >>> case and failing. >> >> >> On a 32 bit architecture, the 64 bit write could easily be split across two >> non-atomic operations. AFAIK there's nothing about the atomic read that says >> that it couldn't observe a partial write by one of the write operations >> inside the mutex. It may well be possible to write a test case to >> demonstrate the issue on a 32-bit ARM machine. And even if it isn't, I would >> much always flag up code like this in a code review as being too subtle and >> prone to potential errors. >> >>> >>> The reason the race detector reports a race is because it expects all >>> access to be guarded by the same guard, the Lock/Release must perform the >>> same memory barriers on the SMP systems I’m aware of. >> >> >> Yes. That's what the memory model expects, and how Go code should be >> written. If code fails when run with the race detector, it should be fixed, >> IMHO. >> >>> >>> The comment on the better “read” method is definitely the way to go. It >>> still encapsulates the channel(s), and avoids any of the ambiguity of the >>> memory model (while probably improving the performance a bit). >>> >>> Agreed that the encapsulation can cause issues like you describe, but the >>> response was originally related to “priority channels”, and I was >>> demonstrating a technique (with additional channels behind the struct) that >>> would allow easy implementations of priorities. But the code I provided >>> certainly simplifies the case of multi writer/readers with indeterministic >>> behavior/shutdown (without using panic/recover which is probably the >>> easiest for the simple case). >>> >>> >>>> On Sep 2, 2019, at 11:16 AM, roger peppe <rogpe...@gmail.com> wrote: >>>> >>>>>> Btw, the removing of the GOMAXPROCS causes things to execute serially- >>>>>> which is fine according to the spec - but it does make demonstrating >>>>>> certain concurrency structs/problems pretty difficult. >>>>>> Unless something's changed recently, all programs in the playground >>>>>> execute without parallelism, so setting GOMAXPROCS shouldn't have any >>>>>> effect. The fact that it does have an affect appears to be a bug. I'd >>>>>> suggest reporting it as an issue. >>>>>> >>>>>> As I pointed out in another reply, I am fairly certain the atomic >>>>>> operations must be valid in these cases due to the happens before >>>>>> relationship of them and mutexes. >>>>>> In that other reply: >>>>>> >>>>>> You can simply validate it by run: go run -race main.go for you program: >>>>>> https://play.golang.org/p/JRSEPU3Uf17 >>>>>> Not true. The race detector does not detect certain cases and can >>>>>> overreport. >>>>>> Firstly, AFAIK there is no implied happens-before relationship between a >>>>>> non-atomic write to a variable and an atomic read from a variable, >>>>>> because there is no synchronization event associated with the atomic >>>>>> read. >>>>>> >>>>>> Secondly, I am aware that the race detector can give false negatives in >>>>>> some cases, but it should not provide false positives AFAIK. There's >>>>>> only one such case that I'm aware of, and that's quite a different issue. >>>>>> >>>>>> Given that the race detector reports a race in this very clear and >>>>>> simple case, ISTM that your program is wrong. >>>>>> >>>>>> From https://golang.org/ref/mem: >>>>>> >>>>>> If you must read the rest of this document to understand the behavior of >>>>>> your program, you are being too clever. >>>>>> >>>>>> Don't be clever. >>>>>> Also, there's really no point in the atomic read. You could implement >>>>>> the Read method as follows: >>>>>> >>>>>> func (c *MultiWriterIntChan) Read() (int, bool) { >>>>>> x, ok := <-c.ch >>>>>> return x, ok >>>>>> } >>>>>> That is, let the close state flow naturally downstream via the channel. >>>>>> That way it will still work correctly if you start to use a buffered >>>>>> channel, for example. >>>>>> >>>>>> I don’t agree that returning the channel is a proper design, it breaks >>>>>> the encapsulation. Technically mine could use multiple channels behind >>>>>> the scenes - yours cannot. >>>>>> There's a trade-off here. By encapsulating in a method, you make it >>>>>> harder to wait for values in combination with other events (you'd need >>>>>> to create a goroutine to call your Read method and send the result to a >>>>>> channel, adding buffering, context switch overhead, and more state that >>>>>> needs to be explicitly shut down). >>>> >>>> >>>> -- >>>> You received this message because you are subscribed to the Google Groups >>>> "golang-nuts" group. >>>> To unsubscribe from this group and stop receiving emails from it, send an >>>> email to golang-nuts+unsubscr...@googlegroups.com. >>>> To view this discussion on the web visit >>>> https://groups.google.com/d/msgid/golang-nuts/CAJhgaci57DOYHOk7C%3DjM8B6wenCxYv4JvNn2%3D13OGcd66Yc3EA%40mail.gmail.com. >>> > -- > You received this message because you are subscribed to the Google Groups > "golang-nuts" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to golang-nuts+unsubscr...@googlegroups.com. > To view this discussion on the web visit > https://groups.google.com/d/msgid/golang-nuts/1753A5A2-5642-49A7-8815-5CC727E67F16%40ix.netcom.com. -- You received this message because you are subscribed to the Google Groups "golang-nuts" group. To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/521AE3D2-2A11-4843-A880-A74C0F33BCE6%40ix.netcom.com.
