Race condition in RPC client's `Stop`?
- Dominant language
- Go
- Stars
- 6.1k
- Forks
- 609
- Avg merge
- 15h 1m
- Merged PRs (30d)
- 3
Description
I might be missing something, and I haven't had the chance to try to create the circumstances where it would trigger, but I'm trying to think through the logic of how `Stop` works in the client, and I think there might be a race condition in it?
1. Suppose client previously subscribed to events with `stream`.
For example let's say `member-join` events with `Seq` value `42`.
2. A `member-join` is sent by the Serf agent to the RPC client.
3. RPC client calls `Stop` with `42`, [de-registering the handler for sequence number 42 as the first thing it does in that call](https://github.com/hashicorp/serf/blob/master/client/rpc_client.go#L684).
4. The `member-join` event from step 2 arrives at the RPC Client's socket.
5. The client reads the `member-join` `Seq` `42` event in `listen`.
6. The `listen` code parses the header of that event (`{"Seq": 42, "Error": ""}`) out of the socket, [tries to handle the event header](https://github.com/hashicorp/serf/blob/master/client/rpc_client.go#L825), but then [discards it because there is no longer a listener](https://github.com/hashicorp/serf/blob/master/client/rpc_client.go#L807).
7. The `listen` code [loops back around to process the next header](https://github.com/hashicorp/serf/blob/master/client/rpc_client.go#L819), **but I think the body of the** `member-join` **is still in the socket** because the handler that would normally read the body from the socket is gone.
8. So I would expect the next loop of `listen` to misparse (because it's expecting another header but getting an event body), log and break out of the loop, effectively killing the RPC client instance.
Seems to me like the way to fix it would be to not de-register the handler for stream/monitor events until after the response for the `stop` command comes back okay.
Contributor guide
Assessment
This issue has not been assessed yet.