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.
