Skip to content

Commit

Permalink
client: Fix deadlock when writing to pipe blocks
Browse files Browse the repository at this point in the history
Swap to calling send, which handles taking sendLock for us rather than
doing it directly in createStream. This means that streamLock is now
released before taking `sendLock`.

Taking sendLock before releasing streamLock means that if a goroutine
blocks writing to the pipe, it can make another goroutine get stuck
trying to take sendLock, and therefore streamLock will be kept locked as
well. This can lead to the receiver goroutine no longer being able to
read responses from the pipe, since it needs to take streamLock when
processing a response. This ultimately leads to a complete deadlock of
the client.

It is reasonable for a server to block writes to the pipe if the client
is not reading responses fast enough. So we can't expect writes to never
block.

I have repro'd the hang with a simple ttrpc client and server. The
client spins up 100 goroutines that spam the server with requests
constantly. After a few seconds of running I can see it hang. I have set
the buffer size for the pipe to 0 to more easily repro, but it would
still be possible to hit with a larger buffer size (just may take a
higher volume of requests or larger payloads).

I also validated that I no longer see the hang with this fix, by leaving
the test client/server running for a few minutes. Obviously not 100%
conclusive, but before I could get a hang within several seconds of
running.

Signed-off-by: Kevin Parsons <[email protected]>
  • Loading branch information
kevpar committed May 11, 2024
1 parent 4a2816b commit f06ddee
Showing 1 changed file with 1 addition and 3 deletions.
4 changes: 1 addition & 3 deletions client.go
Original file line number Diff line number Diff line change
Expand Up @@ -402,11 +402,9 @@ func (c *Client) createStream(flags uint8, b []byte) (*stream, error) {
c.streams[s.id] = s
c.nextStreamID = c.nextStreamID + 2

c.sendLock.Lock()
defer c.sendLock.Unlock()
c.streamLock.Unlock()

if err := c.channel.send(uint32(s.id), messageTypeRequest, flags, b); err != nil {
if err := c.send(uint32(s.id), messageTypeRequest, flags, b); err != nil {
return s, filterCloseErr(err)
}

Expand Down

0 comments on commit f06ddee

Please sign in to comment.