feat: Create config regardless of connection failures #179
Conversation
The channel will try to connect to the node for each message if a connection has not yet been established or if the node has disconnected.
meling
changed the title
There was a problem hiding this comment.
Thanks for this nice work! I've reviewed this without any experimentation; our current tests are passing. I would feel much more confident about approving this if you could add some tests that will fail on the current master branch, but passes with this PR.
Some of my comments are nits that aren't important for the semantics, but will help to improve code quality and style.
The only code change is the move of "if conn == nil" from connect() to tryConnect(). It seems more logical to have this "guard check" at the top of a method, rather than right after starting the sendMsgs goroutine. There should be no semantic differences here, but who knows... this code is a bit intricate. Also adds some doc comment above some (internal) functions, mainly for documentation purposes and some language polish and typos.
Added a guardclause to prevent goroutines being started more than once. Also, the channel will only lock when trying to recreate a nodestream, making it possible to retry when sending messages.
|
Have added a few tests and more documentation. There are now two ways in which the channel will try to reconnect to a node:
Had to add adjust when locking happened in c.reconnect() since sendMsgs and recvMsgs are running in two different goroutines. Now, it will only lock when creating the NodeStream. Regarding the tests: EDIT: The error is coming from encoding.go:113 |
| backoffCfg := c.backoffCfg | ||
|
|
||
| var retries float64 | ||
| for { | ||
| var err error | ||
|
|
||
| c.streamMut.Lock() |
There was a problem hiding this comment.
There is some overlap in logic between this code and the tryConnect method. I wonder if we could make this into a separate method? It is not super obvious though. Still thinking about it.
| @@ -0,0 +1,10 @@ | |||
| syntax = "proto3"; | |||
There was a problem hiding this comment.
Consider reusing tests/dummy/dummy.proto instead; feel free to add message types to that file. Alternatively, you may rename it to mock if that is more appropriate. You should add/update the tests/Makefile to compile the proto file if mock is the name we go with.
This will conflict with PR #179, but should be easy to resolve.
We can avoid passing the context to channel, since we anyway have access to the relevant RawNode.
Since we now cancel pending messages, we have introduced a data race that doesn't appear to have been triggered before.
To avoid having to support another test function, let's make it unexported for now. Currently, we only use it in one place; maybe we can harmonize it with some other helper.
It does not appear to be needed.
A config can be created regardless of a connection to the nodes have failed or not. Instead the channel will try to connect (only once) to the node for each message.
The channel needs to know whether a connection has been established. If not, it will dial the node. Otherwise, it will try to create a stream to the node.