Paasio issues

[exercism]

Paasio issues

[petertseng]

the whole Write function is not thread-safe

Well, this problem was long before my time, but if I look at it, I say yes it's true that Write is not thread safe... which actually is written in the documentation for it:

https://github.com/exercism/xgo/blob/d895e12bc457dad8c024ad457380ade2a5f55aa9/exercises/paasio/example.go#L11
https://github.com/exercism/xgo/blob/d895e12bc457dad8c024ad457380ade2a5f55aa9/exercises/paasio/example.go#L20
https://github.com/exercism/xgo/blob/d895e12bc457dad8c024ad457380ade2a5f55aa9/exercises/paasio/example.go#L30

Furthermore if this function is going to have race problems they're most likely going to be in the buffer write since incrementing an integer is so quick and costs so little time relatively to the buffer write that you need really bad luck to hit it.

Well, the tests do check that the counters are correct. And if you remove the lock/unlock, you actually get incorrect results. There are so many goroutines doing the reads/writes simultaneously. So we have many chances to get really bad luck... so it turns out we actually need really good luck to not see errors.

Putting the wc.w.Write within the critical section is possible

Yeah so as I understand it, it is altogether likely that the contents written to the buffer are complete garbage because of it not being in the critical section.

So I guess the tests are kind of saying "even though the calls to Write() are not synchronized and therefore the customer who used Write() might have written garbage data, you still need to charge your customer for all the garbage data that was written.".

Is that an OK thing to say? I don't know for sure. It's possible that we could make that clear in the docs. For example, to change "calls to Write() are not synchronized" to something like "if multiple goroutines call Write() simultaneously, no guarantee is made about the contents written to the underlying Writer (unless that Writer provides any such guarantees), but the counters will still correctly reflect all operations performed."

as I understand the point of the created code is to provide io.Reader and io.Writer wrappers that themselve satisfy io.Reader and io.Writer respectively. In that respect it's a bit odd that the Read and Write methods would be thread-safe if the interface doesn't require it.

I think that if you are implementing an interface there's nothing wrong with providing extra guarantees above and beyond what the interface requires of you. Should probably document such extra guarantees though (which is why I argue to change the documentation on the functions).

But fundamentally I agree that there is no need for our WriteCounter, etc. to synchronise access to their Writers.

Another issue I see with this code is that it's about mutexes and those are somewhat discouraged in Go. They're around for when you need them but you shouldn't use them unless you have a good reason for why channels won't do the job. So is this something we want to teach to newbies?

I'm a little undecided. Sure, we could have all the counters send byte counts onto a channel and have a single goroutine that reads out of the channel and updates the counter appropriately. No mutex needed then. One disadvantage of this approach I see is that if we no longer need a Reader, we now have no way of telling the goroutine to exit, so it's forced to stick around forever. There's no Close method on these readers. Now we could solve this problem by adding Close methods...

I may take a stab at implementing the channel-based approach and see how it looks.

[petertseng]

As you may have seen, I decided it was best to let the code do the talking, so I implemented a channel-based version of this exercise at #236 and gave my comments there. My general opinion is that for this specific use case I think mutexes are more appropriate than channels and I think we shouldn't be afraid to show that to learners, but I'd gladly take discussion there.

[petertseng]

Given a few factors:

• The concurrency guarantees are now documented
• I believe the mutexes are OK
• Issue's pretty old

I think we have done all we can here and can close, but obviously further discussion is always welcome, here or in another ticket.

[kytrinyx]

Sounds good.