RTN15h*: Handle incoming DISCONNECTED while CONNECTED #179
Conversation
It's a predicate, so the name should reflect that.
This opens the door for RTN15h1, which will share some logic with the handling of FAILED but doesn't have a FAILED protocol message. This shared logic has been moved to an external method.
RTN15h happens in the context of an incoming DISCONNECTED, not FAILED. failedConnSideEffects will be called only when we're sure we're moving to FAILED.
Adds handling of both success and failure of such reconnection.
We removed it from the inner connectWith method because now we can reconnect while already in state CONNECTED, but we do need the check in case the user calls Connect explicitly.
…e/rtn15h-recv-disconnected
| var change ably.ConnectionStateChange | ||
| ablytest.Instantly.Recv(t, &change, stateChanges, t.Fatalf) | ||
|
|
||
| if change.Event != ably.ConnectionEventUpdated { |
There was a problem hiding this comment.
The test is failing with a different event here
--- FAIL: TestRealtimeConn_RTN15h2_Success (0.00s)
realtime_conn_spec_test.go:1230: expected UPDATED event; got {DISCONNECTED DISCONNECTED CONNECTED [ErrorInfo :timeout code=80003 statusCode=0] See https://help.ably.io/error/80003}
There was a problem hiding this comment.
This and similar intermittent errors should've been fixed by 324acf8.
Co-authored-by: Geofrey Ernest <geofreyernest@live.com>
…e/rtn15h-recv-disconnected
Some tests were intermittently failing because of usage of real time by MessagePipe, while the test used mocked time. Those tests aren't interested in timeouts anyway, so do it only when we're explicitly interested.
All the un/locking was causing partially inconsistent states to be observed by tests (and, potentially, by the library itself). Also, fix test so that we don't read msgSerial from a client whose connection has been recovered by another client; it's not defined what that means.
| // we are setting msgSerial as per (RTN16f) | ||
| msgSerial, err := strconv.ParseInt(strings.Split(c.opts.Recover, ":")[2], 10, 64) | ||
| if err != nil { | ||
| //TODO: how to handle this? Panic? |
There was a problem hiding this comment.
We will need to figure out our logging situation at some point. I think implementing the spec is more important at the moment, so its fine if this TODO stays.
There was a problem hiding this comment.
I just moved this code around, the TODO was already there.
Anyway, I agree we should improve logging (see #160) once the codebase becomes less of a moving target.
In this particular case, though, I think a panic is in order. A malformed msgSerial just shouldn't ever happen, and it's already panicking if it doesn't have at least two parts separated by :.
There was a problem hiding this comment.
I see, if that is a case you can just as well add a panic here as part of this PR. We don't need to keep that TODO anymore.
…e/rtn15h-recv-disconnected
Implements RTN15h*.