Deadlock with multiple simultaneous requests from the same client #72
Comments
|
We encountered this with our containerd shim on Windows, where we use a single shim for every container in a pod. It was hit when publishing multiple events back to containerd via TTRPC simultaneously. |
This pulls in a new version of github.com/containerd/ttrpc from a fork to fix the deadlock issue in containerd/ttrpc#72. Will revert back to the upstream ttrpc vendor once the fix is merged (containerd/ttrpc#73). Signed-off-by: Kevin Parsons <[email protected]>
This pulls in a new version of github.com/containerd/ttrpc from a fork to fix the deadlock issue in containerd/ttrpc#72. Will revert back to the upstream ttrpc vendor once the fix is merged (containerd/ttrpc#73). This fix also included some vendoring cleanup from running "vndr". Signed-off-by: Kevin Parsons <[email protected]>
Changes the TTRPC client logic so that sending and receiving with the server are in completely independent goroutines, with shared state guarded by a mutex. Previously, sending/receiving were tied together by reliance on a coordinator goroutine. This led to issues where if the server was not reading from the connection, the client could get stuck sending a request, causing the client to not read responses from the server. See [1] for more details. The new design sets up separate sending/receiving goroutines. These share state in the form of the set of active calls that have been made to the server. This state is encapsulated in the callMap type and access is guarded by a mutex. The main event loop in `run` previously handled a lot of state management for the client. Now that most state is tracked by the callMap, it mostly exists to notice when the client is closed and take appropriate action to clean up. [1] containerd#72 Signed-off-by: Kevin Parsons <[email protected]>
Changes the TTRPC client logic so that sending and receiving with the server are in completely independent goroutines, with shared state guarded by a mutex. Previously, sending/receiving were tied together by reliance on a coordinator goroutine. This led to issues where if the server was not reading from the connection, the client could get stuck sending a request, causing the client to not read responses from the server. See [1] for more details. The new design sets up separate sending/receiving goroutines. These share state in the form of the set of active calls that have been made to the server. This state is encapsulated in the callMap type and access is guarded by a mutex. The main event loop in `run` previously handled a lot of state management for the client. Now that most state is tracked by the callMap, it mostly exists to notice when the client is closed and take appropriate action to clean up. Also did some minor code cleanup. For instance, the code was previously written to support multiple receiver goroutines, though this was not actually used. I've removed this for now, since the code is simpler this way, and it's easy to add back if we actually need it in the future. [1] containerd#72 Signed-off-by: Kevin Parsons <[email protected]>
TestMultipleVMs_Isolated is still unstable (see firecracker-microvm#581). Apparently having multiple simultaneous requests from the same client is known to be problematic (see containerd/ttrpc#72). This commit workarounds the issue by making a client per VM. Signed-off-by: Kazuyoshi Kato <[email protected]>
|
What's the status on this? |
|
I see @kzys comment now 🤦 #72 (comment) |
|
I recently had reason to dig into this again. Originally, the deadlock issue was fixed via #94.
Together, these function fine together, and there are no (known to me) deadlocks. Then, everything changed when #107 was merged.
With #107, there are no deadlocks as long as both client and server are using post-#107 versions. However, if the server is pre-#107, and the client is post #107 (or pre-#94), there is once again a chance of deadlock. More broadly, a robust client should not rely on specific server implementation details to avoid deadlocking. We must ensure the client can never block receiving responses if request writing is blocked on the pipe. The specifics leading to the deadlock are as follows: There is shared state in the client between sending requests and receiving responses. This state is
https://github.com/containerd/ttrpc/blob/main/client.go#L405-L407 The reason for the deadlock is the ordering of steps 2 and 3 above. Because
At this point, the only way they can make progress is for Goroutine 1's write to the pipe to complete. But as we said above, it's reasonable for the server to not keep reading requests if the client is not reading responses quickly enough! Luckily, the fix is straightforward. We just need to flip steps 2 and 3, so that we release 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). @kiashok FYI |
|
I have published #168 with a fix for this. |
|
This is now fixed in v1.2.4. |
There is a possible deadlock in the TTRPC server/client interactions when there are multiple simultaneous requests from the same client connection. This causes both the server and client handler goroutines to deadlock.
I've repro'd this on both Linux (with unix sockets as the transport) and Windows (with both unix sockets and named pipes as the transport). It repros more easily when the transport has less buffering, and when there are more goroutines sending requests concurrently from the client.
I intend to look into how this can be fixed, but filing an issue for awareness and in case someone else wants to tackle it in the meantime. :)
Stacks
Server
Client
Analysis
Basically, the server has a "receiver" goroutine that receives new requests from the transport, and sends a message via channel to the "worker" goroutine. The "worker" goroutine has a loop and a
selectto handle either a new request message, or a response that needs to be sent to the client. When the deadlock occurs, the server is stuck blocking on a response write to the transport from the "worker" goroutine, while the "receiver" goroutine is stuck trying to send a request message to the "worker" goroutine.The client side is basically the inverse of this, where the "receiver" goroutine is stuck trying to send a response message received on the transport to the "worker" goroutine via channel. The "worker" goroutine is likewise stuck trying to send a new request to the server via the transport.
This looks like it should only occur when the connection is busy enough that the transport buffer is filled, as otherwise the server and client writes to the transport would simply be fulfilled by the buffer, and would not block waiting for a reader on the other end.
The interesting places in the code where the 4 goroutines are stuck are linked below:
Server receiver sending message to worker: https://github.com/containerd/ttrpc/blob/v1.0.2/server.go#L404
Server worker writing response to transport: https://github.com/containerd/ttrpc/blob/v1.0.2/server.go#L459
Client receiver sending message to worker: https://github.com/containerd/ttrpc/blob/v1.0.2/client.go#L222
Client worker writing request to transport: https://github.com/containerd/ttrpc/blob/v1.0.2/client.go#L273
Sample
I have a repro program here. This program can be run as either a server (
go run . server) or client (go run . client). The server implements a very simple TTRPC server that listens for connections. The client spawns multiple goroutines to constantly send requests to the server and print their ID each time they get a response. Each request/response has a bunch of junk data added to the message to try to avoid the affects of buffering on the underlying transport. When run, you will generally see a little bit of output from the client, but then it will stop when the deadlock occurs. You can also hit enter on either the server or client to cause them to dump their current goroutine stacks to a file.The text was updated successfully, but these errors were encountered: