add auto-reconnect while still returning a timeout error #2
Conversation
|
This is an alternative implementation to Roasbeef/btcwallet#21 |
| c.timeout) | ||
| if err != nil { | ||
| // Prevent CPU overuse by refused reconnection attempts. | ||
| time.Sleep(c.timeout) |
There was a problem hiding this comment.
Perhaps we should use an exponential back off here? For reference the btcsuite rpcclient does something similar: https://github.com/btcsuite/btcd/blob/master/rpcclient/infrastructure.go#L641
There was a problem hiding this comment.
This is used synchronously, which means each time we back off, we can delay processing of other events in an event loop that include a call to this function. For example, https://github.com/Roasbeef/btcwallet/blob/master/chain/bitcoind.go#L632 is in a loop that also checks for updates to watch lists and rescans. The rpcclient reconnect handler runs as a goroutine.
An option for doing the incremental back off would be to do that in a goroutine, and just sleep/return a timeout error in the synchronous portion.
There was a problem hiding this comment.
Another option for doing incremental backoff is to add more state to Conn to keep track of last attempted reconnection time when disconnected.
There was a problem hiding this comment.
I see, so even with this patch, we need reconnection logic within the wallet as well? As this just return a timeout error in either case, even if we're able to reconnect successfully.
There was a problem hiding this comment.
No, the wallet already ignores read timeouts, this code just emulates those: https://github.com/Roasbeef/btcwallet/blob/master/chain/bitcoind.go#L636
There was a problem hiding this comment.
If we reconnect successfully, the next attempt at reading will return a message if there is one.
There was a problem hiding this comment.
Ahh, thanks! OK, will test out this patch locally.
| @@ -50,6 +62,7 @@ func connFromAddr(addr string) (net.Conn, error) { | |||
| // Conn is a connection to a ZMQ server. | |||
| type Conn struct { | |||
| conn net.Conn | |||
| topics []string | |||
There was a problem hiding this comment.
I don't see this value ever being set. So it would see that with this, we'd be able to reconnect, but then would lose notifications concerning the topics we care about?
| @@ -326,7 +339,7 @@ func Subscribe(addr string, topics []string, timeout time.Duration) (*Conn, erro | |||
|
|
|||
| conn.SetDeadline(time.Now().Add(10 * time.Second)) | |||
|
|
|||
| c := &Conn{conn, timeout} | |||
| c := &Conn{conn, topics, timeout} | |||
This allows the ZMQ SUB to automatically reconnect to a PUB socket when disconnected. It hides the disconnection and reconnection from the client; therefore, in use cases like
btcwallet, there will be no logging of the disconnect/reconnect.