hashicorp / hashicorp/serf

Race condition in RPC client's `Stop`?

Open
#552 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.