Fix race condition on early connection failure #1430
Conversation
Both `Client` and `TransportHandler` were watching the connection actor, which resulted in undeterministic behavior during termination of `PeerConnection`. We now always return a message when a connection fails during authentication. Took the opportunity to add more typing (insert deathtoallthestring.jpg).
| @@ -112,24 +115,28 @@ class Client(nodeParams: NodeParams, switchboard: ActorRef, router: ActorRef, re | |||
| SupervisorStrategy.Stop | |||
| } | |||
|
|
|||
|
|
|||
| override def postStop(): Unit = { | |||
| context stop self | ||
| case Socks5Connected(_) => | ||
| log.info(s"connected to ${str(remoteAddress)} via SOCKS5 proxy ${str(proxyAddress)}") | ||
| auth(proxy) | ||
| context become connected(proxy) | ||
| context unwatch proxy |
There was a problem hiding this comment.
What happens to the proxy then? We're now calling auth on connection instead of proxy, is that ok?
There was a problem hiding this comment.
Oops, fixed with a113b8c.
We are obviously missing a test, but it is easier said than done. Would need to mock a socks5 proxy?
There was a problem hiding this comment.
It's not easy, if we simply mock there's a risk our mock deviates from what the real proxy does and our test isn't accurate...it's enough of an edge case that I think we're ok with what we have for now
There was a problem hiding this comment.
Maybe we should deploy an Acinq node behind Tor, with a small amount of funds, just to make sure we test this E2E often enough?
There was a problem hiding this comment.
We do, both on testnet and mainnet. But our peers can initiate connections too, so this bugs is not easy to detect. It also means it doesn't have a lot of impact.
will monitor on next deployment |
Both
ClientandTransportHandlerwere watching the connection actor,which resulted in undeterministic behavior during termination of
PeerConnection.We now always return a message when a connection fails during
authentication.
Took the opportunity to add more typing (insert
deathtoallthestring.jpg).