Skip to content

Commit

Permalink
docs: stream Reads are not safe for concurrent use (hashicorp#140)
Browse files Browse the repository at this point in the history
In hashicorp#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: hashicorp#139
  • Loading branch information
tgross authored Dec 4, 2024
1 parent 4bc3fc7 commit 060257c
Showing 1 changed file with 11 additions and 8 deletions.
19 changes: 11 additions & 8 deletions stream.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,10 @@ const (
streamReset
)

// Stream is used to represent a logical stream
// within a session.
// Stream is used to represent a logical stream within a session. Methods on
// Stream are safe to call concurrently with one another, but all Read calls
// must be on the same goroutine and all Write calls must be on the same
// goroutine.
type Stream struct {
recvWindow uint32
sendWindow uint32
Expand Down Expand Up @@ -92,8 +94,10 @@ func (s *Stream) StreamID() uint32 {
}

// Read is used to read from the stream. It is safe to call Write, Read, and/or
// Close concurrently but Stream provides no guarantees that concurrent Reads
// will receive data in response to Writes made from the same goroutine.
// Close concurrently with each other, but calls to Read are not reentrant and
// should not be called from multiple goroutines. Multiple Read goroutines would
// receive different chunks of data from the Stream and be unable to reassemble
// them in order or along message boundaries, and may encounter deadlocks.
func (s *Stream) Read(b []byte) (n int, err error) {
defer asyncNotify(s.recvNotifyCh)
START:
Expand Down Expand Up @@ -161,8 +165,8 @@ WAIT:
}

// Write is used to write to the stream. It is safe to call Write, Read, and/or
// Close concurrently but Stream provides no guarantees that concurrent Reads
// will receive data in response to Writes made from the same goroutine.
// Close concurrently with each other, but calls to Write are not reentrant and
// should not be called from multiple goroutines.
func (s *Stream) Write(b []byte) (n int, err error) {
s.sendLock.Lock()
defer s.sendLock.Unlock()
Expand Down Expand Up @@ -324,8 +328,7 @@ func (s *Stream) sendClose() error {
return nil
}

// Close is used to close the stream. It is safe to call Write, Read, and/or
// Close concurrently.
// Close is used to close the stream. It is safe to call Close concurrently.
func (s *Stream) Close() error {
closeStream := false
s.stateLock.Lock()
Expand Down

0 comments on commit 060257c

Please sign in to comment.