Reset timeout when we get a message and cleanup connect internal code#12
Reset timeout when we get a message and cleanup connect internal code#12masad-frost merged 6 commits intomasterfrom
Conversation
Much less indirection. Does not include timeouts
we changed the way it works a bit in that if we ever get a message on the socket we will reset the timeout
| * @asMemberOf Channel | ||
| * @event | ||
| */ | ||
| declare function close(c: { closeEvent?: CloseEvent; expected: boolean }): void; |
There was a problem hiding this comment.
Just typing the close event
| @@ -420,61 +386,38 @@ export class Client extends EventEmitter { | |||
| }; | |||
|
|
|||
| private onClose = ({ closeEvent, expected }: { closeEvent?: CloseEvent; expected: boolean }) => { | |||
There was a problem hiding this comment.
This kind of does the same thing except that it now is never called directly by the client unless we've had a successful connection.
The websocket cleanup is split up into a separate method so that we can call it when the connect function is rejected
| * The user might call `close` before we even connect | ||
| * we wanna make sure we reject the promise if that happens | ||
| * so we monkey patch our own `close` function ;) | ||
| */ | ||
| const originalClose = this.close; | ||
| this.close = () => { | ||
| this.debug({ type: 'breadcrumb', message: 'user close' }); | ||
| onFailed(new Error('You called `Client.close` before you connected')); | ||
| }; |
There was a problem hiding this comment.
Feels like we can do better in this case, but I'm not sure what.
| /** | ||
| * success is only called when we get | ||
| * ContainerState.READY command | ||
| */ | ||
| let onSuccess: () => void; | ||
| /** | ||
| * Failure can happen due to a number of reasons | ||
| * 1- Abrupt socket closure | ||
| * 2- Timedout connection request | ||
| * 3- ContainerState.SLEEP command | ||
| * 4- Use calling `close` before we connect | ||
| */ | ||
| let onFailed: (err: Error) => void; |
There was a problem hiding this comment.
These methods are assigned at the end of the function (inside the promise constructor), they are referenced in the code before they are assigned (see ws.onclose for example), but since all the ways they can be called is async that shouldn't be a problem! LMK if I'm wrong
|
unbooping: approved |
3 participants
Why
We started sending the status of the container startup from the backend when we start a connection. After discussing with the infra team (aka mason), it made sense to reset the connection timeout as long as we're receiving something from the backend, which signifies that the connection will eventually work.
While I was implementing the above, I realized how shit the code is for
tryConnectwhich is the internal connect function, it had so much indirection and wasn't the most enjoyable to read, so I refactored it. I guess I could've implemented the timeout reset feature on a separate branch but the code was too smelly 💩What changed
First thing I did I got rid of
deferredReadyfrom the client.deferredReadywas used all over the client code because we would resolve/reject the connection promise from multiple places (indirection).Instead, now we localized all the code for resolving the promise within the
tryConnectfunction itself. We're still dealing with event emitters (the websocket and channels) so it feels like there's some indirection but it's a bit more natural now since everything is colocated in the same function.I added code comments so you can follow along the
tryConnectfunction. I'll add more info this PR as well.Test plan
Try all the possible paths of failure, they all should work.
We really need to write unit tests for this as it's easily testable!