Ok, I think I understand this now.

The Ping is needed to actually detect if the connection went down. Looks like internal TCP keepalive mechanics are not responsive / platform-independent enough, so having some kind of ping/heartbeat like this is recommended:

https://stackoverflow.com/questions/865987/do-i-need-to-heartbeat-to-keep-a-tcp-connection-open
https://felixge.de/2014/08/26/tcp-keepalive-with-golang.html
and this one has much more detail: http://250bpm.com/blog:22

Problem is the Ping is happening in an independent go-routine, so it could happen concurrently with a call to SignVote or SignProposal. So we need the mtx in the RemoteSignerClient so that it's thread safe.

An alternative might be to serialize the pings and writes using channels, so only a single go-routine is ever writing to the connection. Then we can remove the mtx. We might want to do that anyways for finer grained control over the ping (ie. we could reset the ping timer every time we receive a msg, which would result in sending far fewer pings).

This issue is not related to #2677, but it is related to #2784, which surfaced a data race because a test was calling RemoteSignerClient functions directly without holding the lock, thus racing with the concurrent Pings.

Replacing with #3107

privval: clarify need for mutex in socket impl #3064

Description

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions