fix: handle removing closed connections#254
Conversation
| if !exists { | ||
| r.outgoingConnectionsLock.RLock() | ||
| outgoingConnection := r.outgoingConnectionsByBroker[url] | ||
| outgoingConnection, outgoingConnectionExists := r.outgoingConnectionsByBroker[url] |
There was a problem hiding this comment.
if the connection is closed on the stork side, the onClose() will get called which removes this connection from this map. So we need to handle the possibility that the connection has already been removed from the map and not call .Remove() in that case
| brokerPublishUrl1: {"BTCUSD": {}}, | ||
| }, nil).Once() | ||
| runner.UpdateBrokerConnections() | ||
| time.Sleep(100 * time.Millisecond) |
There was a problem hiding this comment.
first create the connections by mocking a registry response and calling update connections
| time.Sleep(100 * time.Millisecond) | ||
|
|
||
| // close connection (disconnected by stork) | ||
| runner.outgoingConnectionsByBroker[brokerPublishUrl1].onClose() |
There was a problem hiding this comment.
call the onClose for this connection (which removes it from the relevant map)
| runner.outgoingConnectionsByBroker[brokerPublishUrl1].onClose() | ||
|
|
||
| // removing the only connection means that we get an auth error when hitting the broker | ||
| registry.On("GetBrokersForPublisher", mock.Anything).Return(nil, errors.New("fake error")).Once() |
There was a problem hiding this comment.
in practice removing the last connection means this returns an error, so model that behavior in test
| // removing the only connection means that we get an auth error when hitting the broker | ||
| registry.On("GetBrokersForPublisher", mock.Anything).Return(nil, errors.New("fake error")).Once() | ||
|
|
||
| runner.UpdateBrokerConnections() |
There was a problem hiding this comment.
this call was panicking before I updated runner.go
2 participants
Change
When a broker connection is removed for a publisher agent, it may be disconnect on stork's side before the publisher agent attempts to remove the connection. We should handle this case and not call
.Remove()if the connection is already closed, since it will cause a nil panic.Testing