Add connection Reconnect method #18
Conversation
|
I will include unit tests if this approach is approved |
|
Alternative approach would be to nil out the c.conn, I felt this was a bit safer as it didnt impact existing code |
connection.go
Outdated
| return fmt.Errorf("connecting to server %s: %w", c.Addr, err) | ||
| } | ||
|
|
||
| c.conn = conn |
There was a problem hiding this comment.
Do we need to worry about concurrent calls to Reconnect?
There was a problem hiding this comment.
If used in a sane fashion? no, since it is a library that clients can do anything with? probably.
probably better idea to guard with a mutex, Ill add
…AR-814-add-reconnect-option
… that it works in concurrency.
ev-the-dev
requested review from
adamdecaf,
alovak,
mhargrove and
ev-the-dev
| require.NoError(t, c.Close()) | ||
| }) | ||
|
|
||
| t.Run("reconnect called on closed connection", func(t *testing.T) { |
There was a problem hiding this comment.
let's add one more test when the connection was closed by the server and we call Reconnect (without calling Close(). I guess this is how we are going to use inside brand-gateway.
Here is how we can ask servier to close the connection:
iso8583-connection/connection_test.go
Line 696 in 28c380d
There was a problem hiding this comment.
Are you referring to triggering <-done before the goroutine reaches it?
|
closing it for now. if we want to return to it, we can re-open PR |
Currently if a connection goes offline, there is no way to re-estalbish this entire connection because the Connect method will see the existing net.Conn object in c.conn on these lines and use the existing (and closed) connection.