fix: Add atomic.Bool to ensure wsConnection.Disconnect() is only called once #359
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #359 +/- ##
==========================================
+ Coverage 78.46% 80.24% +1.77%
==========================================
Files 25 26 +1
Lines 2419 2500 +81
==========================================
+ Hits 1898 2006 +108
+ Misses 413 383 -30
- Partials 108 111 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
server/serverimpl_test.go
Outdated
| require.Equal(t, 1, len(logger.logs)) | ||
| require.Contains(t, logger.logs[0], "Errorf: Cannot read a message from WebSocket: read tcp") | ||
| require.Contains(t, logger.logs[0], "use of closed network connection") |
There was a problem hiding this comment.
Do we care about the log messages produced? It looks fragile to make the tests dependent on the exact formatting of the messages.
There was a problem hiding this comment.
Agree we should avoid fragility. The original motivation for this change was to deal with superfluous error logs produced by the server-side Disconnect() calls. Open to suggestions for a better criteria for to test for.
There was a problem hiding this comment.
Perhaps as a compromise: in testLogger store error logs separate from debug logs and here only assert that there is an error log produced, but don't check the error message itself. This should be fairly stable since we always expect exactly one error log and don't really care about the message itself.
There was a problem hiding this comment.
Sounds like a good compromise. All set.
|
@shazlehu Is this still in progress? |
|
@tigrannajaryan I'll make a push to get this over the finish line in the next week |
There was a problem hiding this comment.
Thanks for working on this @shazlehu
Mostly LGTM, just 2 minor comments.
server/wsconnection.go
Outdated
| } | ||
|
|
||
| var _ types.Connection = (*wsConnection)(nil) | ||
|
|
||
| func (c wsConnection) Connection() net.Conn { | ||
| func newWSConnection(wsConn *websocket.Conn) types.Connection { | ||
| return &wsConnection{wsConn: wsConn, connMutex: sync.Mutex{}, closed: atomic.Bool{}} |
There was a problem hiding this comment.
I think zero-initialization for connMutex and closed is fine, e.g. this should work the same way and is less verbose:
| return &wsConnection{wsConn: wsConn, connMutex: sync.Mutex{}, closed: atomic.Bool{}} | |
| return &wsConnection{wsConn: wsConn} |
server/serverimpl_test.go
Outdated
| require.Equal(t, 1, len(logger.logs)) | ||
| require.Contains(t, logger.logs[0], "Errorf: Cannot read a message from WebSocket: read tcp") | ||
| require.Contains(t, logger.logs[0], "use of closed network connection") |
There was a problem hiding this comment.
Perhaps as a compromise: in testLogger store error logs separate from debug logs and here only assert that there is an error log produced, but don't check the error message itself. This should be fairly stable since we always expect exactly one error log and don't really care about the message itself.
… wsConnection intialization
shazlehu
force-pushed
the
fix/extra-error-log
branch
from
4a52657 to
b30f020
Compare
Addresses this issue by addding an atomic.Bool so we only close the underlying websocket connection once. This PR also adds the test
TestDisconnectServerWSConnectionto ensure that that error log isn't generated when the connection is closed server-side. I renamedTestDisconnectWSConnectiontoTestDisconnectClientWSConnectionsince it tests closing the connection from the client-side, though the comments in the test implied it tested disconnecting from the server.Perhaps it would be better to simply ignore the error when we close the connection twice? I'm unsure if there are error conditions where we'd want to be able to try to close the connection again.