feat: Supports callbacks when reading a message fails #331
Conversation
|
|
tttoad
force-pushed
the
feat-error-cb
branch
from
cbc48e1 to
2ed37ef
Compare
|
Where is the new callback used? |
Sorry, missed submitting a file change. |
|
This conflicts with other changes to callbacks. Please recreate if still relevant. |
server/serverimpl.go
Outdated
| var ( | ||
| errAlreadyStarted = errors.New("already started") | ||
| ) | ||
| var errAlreadyStarted = errors.New("already started") |
There was a problem hiding this comment.
Please don't make irrelevant changes.
server/types/callbacks.go
Outdated
| @@ -62,6 +62,9 @@ type ConnectionCallbacks struct { | |||
|
|
|||
| // OnConnectionClose is called when the OpAMP connection is closed. | |||
| OnConnectionClose func(conn Connection) | |||
|
|
|||
| // OnConnectionError is called when an error occurs while reading or serializing a message. | |||
There was a problem hiding this comment.
| // OnConnectionError is called when an error occurs while reading or serializing a message. | |
| // OnReadMessageError is called when an error occurs while reading or deserializing a message. |
server/serverimpl_test.go
Outdated
| defer conn.Close() | ||
|
|
||
| // Send a message to the Server. | ||
| err := conn.WriteMessage(websocket.TextMessage, []byte("")) |
There was a problem hiding this comment.
Include some payload to verify it gets delivered to the callback ,e.g.:
| err := conn.WriteMessage(websocket.TextMessage, []byte("")) | |
| err := conn.WriteMessage(websocket.TextMessage, []byte("abc")) |
server/serverimpl_test.go
Outdated
| eventually(t, func() bool { return rcvMsg.Load() != nil }) | ||
| errInfo := rcvMsg.Load().(ErrorInfo) | ||
| assert.EqualValues(t, websocket.TextMessage, errInfo.mt) | ||
| assert.EqualValues(t, []byte(""), errInfo.msgByte) |
There was a problem hiding this comment.
| assert.EqualValues(t, []byte(""), errInfo.msgByte) | |
| assert.EqualValues(t, []byte("abc"), errInfo.msgByte) |
| if err != nil { | ||
| s.logger.Errorf(msgContext, "Cannot decode message from WebSocket: %v", err) | ||
| connectionCallbacks.OnReadMessageError(agentConn, mt, msgBytes, err) |
There was a problem hiding this comment.
In addition, do we want OnReadMessageError to return a bool/enum, indicating that the connection must be kept or terminated? This gives additional control to the callback owner, for example to inspect the error and decide that it doesn't want to keep receiving messages from clients that send invalid messages.
I would expect strict serves to prefer this behavior, since it is likely that a misbehaving client will keep misbehaving and we would rather cut the communication early.
Since we are introducing a new callback in the public API, I'd rather make it more universally useful for a more than one case if we can, not just for logging (at a cost of tiny extra complexity of returning and checking a value from callback).
@andykellr thoughts?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #331 +/- ##
==========================================
- Coverage 78.29% 77.92% -0.37%
==========================================
Files 25 25
Lines 2386 2401 +15
==========================================
+ Hits 1868 1871 +3
- Misses 410 420 +10
- Partials 108 110 +2 ☔ View full report in Codecov by Sentry. |
tttoad
force-pushed
the
feat-error-cb
branch
from
1b1915e to
8335e42
Compare
see: #330