Fix deadlock when writing to pipe blocks

[kevpar]

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.

[cpuguy83]

LGTM

The old behavior does indeed look fishy as generally locks need to be taken and released in order.
I guess the main thing here is the message may send out of order now.
I don't think that should matter.

[cpuguy83]

CI appears to have hung, however.

[kiashok]

@dmcgowan could you take a look? This deadlock seems to be happening quite a bit offlate for some customers.

[dmcgowan]

it can make another goroutine get stuck trying to take sendLock

Where is the other sendLock hung at? The stream creation ordering must still be preserved and unlocking streamlock before the send lock opens up the possibility of a protocol error sending out of order stream creates. While that is unlikely, it must be protected and would probably be even more difficult to reproduce.

[kevpar]

it can make another goroutine get stuck trying to take sendLock

Where is the other sendLock hung at? The stream creation ordering must still be preserved and unlocking streamlock before the send lock opens up the possibility of a protocol error sending out of order stream creates. While that is unlikely, it must be protected and would probably be even more difficult to reproduce.

Goroutine 1 is holding sendLock and is blocked sending a request on the pipe (https://github.com/containerd/ttrpc/blob/main/client.go#L409).
Goroutine 2 is holding streamLock and is blocked trying to take sendLock (https://github.com/containerd/ttrpc/blob/main/client.go#L405).
Goroutine 3 is the client's receiver goroutine, and is blocked trying to take streamLock as part of processing an incoming response (https://github.com/containerd/ttrpc/blob/main/client.go#L424).

I put more info here too: #72 (comment)

What is the reason that stream IDs need to be sent over the wire in order? Could we relax that constraint?

[dmcgowan]

Its seems in that case, goroutine 3 should be able to proceed if we managed the locking better. Goroutine 1 and 2 are correctly blocking each other.

I think putting the back pressure on the client would be good, but we definitely shouldn't have any contention between requests and response in the client. I think if we broke up streamLock from an rwmutex into two mutexes it would be cleaner. The first mutex (or could just be any synced map) around access to s.streams, then the second mutex only on createStream to protect the stream id ordering. Its good to avoid RWMutex anyway, but in this case a single RWMutex doesn't seem correct and it covering too much.

What is the reason that stream IDs need to be sent over the wire in order? Could we relax that constraint?

This design we borrowed from spdy/http2 which has pretty solid reasoning behind it. It keeps the stream ids unique without worrying about clashing or tracking a large map of active or previously seen identifiers. This make the implementation easy and fast, you only need a single incrementing integer.

[kevpar]

I think putting the back pressure on the client would be good, but we definitely shouldn't have any contention between requests and response in the client. I think if we broke up streamLock from an rwmutex into two mutexes it would be cleaner. The first mutex (or could just be any synced map) around access to s.streams, then the second mutex only on createStream to protect the stream id ordering. Its good to avoid RWMutex anyway, but in this case a single RWMutex doesn't seem correct and it covering too much.

I think it works to just take sendLock before we take streamLock in createStream and hold it for the entire function. I just pushed a change that does this, can you take a look?

I'm still reviewing it to make sure it doesn't seem like it will break anything else, but this appears good as streamLock is the only contention between sending and receiving, and it's never held across any blocking calls. Taking sendLock at the start of createStream should ensure we still send new stream IDs in correct order.

We could also swap from streamLock + map to a sync.Map. I'm fine either way.

[dmcgowan]

Even better!

We can leave the stream lock alone in this PR but switching to sync.Mutex + map or sync.Map is probably better than using RWMutex.

[kevpar]

Sounds great! I will fix up the commit description to be accurate, and look over a bit more for any concerns. Otherwise I think this should be good.

[kevpar]

It's possible adding another mutex (to have separate mutexes for "write to the wire" and "allocate stream + write to the wire") would reduce contention slightly, since (*stream).send would only need to the take first mutex. However, stream allocation should be very fast since there are no blocking calls, and I'm not sure if it's worth the extra code complexity of adding another mutex into the mix.

[kevpar]

Thanks for the quick review!