Add state machine to ReconnectingWebSocket#744
Conversation
mafredri
left a comment
There was a problem hiding this comment.
Nice refactor. Added some comments and questions, but the general theme I see that could be improved upon is the robustness of maintaining the correct state during asynchronous code execution. Right now it seems like there are a few minor edge cases where the state machine might not be fully protected against an unintended transition.
Some suggestions to tighten up the state machine include:
- Ensure there's fewer (or ideally one) code-path for state transitions.
- Track connection ID in async handlers to prevent stale transitions.
PS. A state machine diagram showing states and their possible transitions may be useful.
| // Check if disconnected/disposed while waiting for factory | ||
| if (this.#isDisposed || this.#isDisconnected) { | ||
| // Check if state changed while waiting for factory (e.g., disconnect/dispose called) | ||
| if (this.#state !== ConnectionState.CONNECTING) { |
There was a problem hiding this comment.
Are we expecting something may asynchronously change this state considering we just set connecting?
There was a problem hiding this comment.
Yes, JS is single threaded but it's async so there could be a case where we await socketFactory and then during that we call reconnect/dispose... etc
67c4fa5 to
a14e131
Compare
a14e131 to
46466c7
Compare
46466c7 to
5042fa5
Compare
Would it make sense to add something like this here: function reduceState(
state: ConnectionState,
action: StateAction,
): ConnectionState {
switch (action.type) {
case "CONNECT":
switch (state) {
case ConnectionState.IDLE:
case ConnectionState.CONNECTED:
case ConnectionState.AWAITING_RETRY:
case ConnectionState.DISCONNECTED:
return ConnectionState.CONNECTING;
default:
return state;
}
...That way it's a true state machine and if the state does not change then we return |
Your suggestion makes sense to me and seems like a simple way to make the state machine easier to follow. 👍🏻 |
Replace direct state assignments with a pure reducer pattern: - Add StateAction type and reduceState() pure function - Add #dispatch() method that validates transitions - Remove #pendingReconnect flag - reconnect() now restarts immediately
5042fa5 to
358d4d0
Compare
|
Implemented a reducer function - BTW the last commit is unrelated but I added it because it was causing WS leaks to happen more frequently (basically only reconnect if we are not connected) |
af8817b to
69df291
Compare
26b7fe0 to
6b4fe99
Compare
Summary
ConnectionStateenum for clearer state managementChanges
Refactored
ReconnectingWebSocketto use a state machine pattern with six states:IDLE- Initial state, ready to connectCONNECTING- Actively running socket factoryCONNECTED- Socket is open and workingAWAITING_RETRY- Waiting for backoff timer before reconnectionDISCONNECTED- Temporarily paused, user must callreconnect()to resumeDISPOSED- Permanently closed, cannot be reusedThis makes the websocket lifecycle easier to understand and debug, and prevents invalid state combinations that could
occur with independent boolean flags.
Closes #754