docs: stream Reads are not safe for concurrent use

[tgross]

In #136 we documented warnings around the semantics of calling Stream.Read concurrently. But we assumed without investigating that the concurrent operations were actually safe. But in fact the locks made by multiple Read calls can interleave, causing a deadlock. Update the documentation again to make this clear.

Fixes: #139

[lukaspj]

Do you think that stream closure can still be safely detected by reading an empty array like this example?
#115 (review)

Or do we need another pattern?

[schmichael]

Do you think that stream closure can still be safely detected by reading an empty array like this example? #115 (review)

Or do we need another pattern?

That pattern is fine as long as you do not expect data or concurrent Reads on that Stream. At least until we fix the deadlock there should only be 1 Reader and 1 Writer per Stream.

That being said, even if we fix the deadlock I can not think of a case where multiple Readers on a single Stream is coherent or at least optimal. Multiple concurrent Readers would receive different chunks of Writes to the Stream, and lose the order they came in which would prevent reassembly. I cannot think of a way that multiple concurrent Readers (with buffers) could produce coherent results.

Your use case of splitting "main" Reads and EOF detection into 2 goroutines doesn't run into coherency issues, but it still isn't optimal as on data the empty-buffer Reader will spin in a tight loop until the main Reader actually Reads the buffer. In practice you may only see 0-2 empty-buffer Reads before the main Reader picks up the data. However, I would suspect in loaded conditions where the main Reader goroutine may spend a significant amount of time performing operations on received data before calling Read again: the empty-buffer Reader may spin quite a lot and waste considerable CPU time.

So my recommended pattern, regardless of deadlock/race-condition bugs, is to always have at most 1 Reader and 1 Writer goroutine for yamux Streams (any goroutine can call Close). Your main Reader should already be detecting and handling EOFs, so hopefully removing the empty-buffer goroutine doesn't complicate your code. You can always keep your EOF-detection goroutine, give it a buffer, and on data copy the buffer over a channel or pipe to the main Reader instead of having the main Reader use the Stream directly.

[lukaspj]

Ah yeah not an issue for us at all, just need to know what we can and can’t do then we’re good 😊
Thanks a lot! Really appreciate all the time you put into this!

[mukeshjc]

LGTM