Add some thrown errors if the at driver has closed #57
Conversation
src/agent/at-driver.js
Outdated
| @@ -62,6 +69,7 @@ export class ATDriver { | |||
| for await (const rawMessage of iterateEmitter(this.socket, 'message', 'close', 'error')) { | |||
| const message = rawMessage.toString(); | |||
| this.log(AgentMessage.AT_DRIVER_COMMS, { direction: 'inbound', message }); | |||
| if (this.hasClosed) throw new Error('AT-Driver connection unexpectedly closed'); | |||
There was a problem hiding this comment.
The design of iterateEmitter suggests that this is not an exceptional circumstance--the utility seems designed to gracefully handle the case where the completEvent (which is "close" in this case) occurs while values are still being emitted.
I honestly don't have any recollection of the thinking that went into this, but from reviewing the source, I believe it may be because values are always emitted asynchronously. It seems possible that close occurs while the utility is still working through its internal backlog of values.
If "close" is an exceptional circumstance in this context, then maybe we should specify null as the completeEvent for this invocation.
What do you think?
There was a problem hiding this comment.
hrm, you seem right, 'close' and 'error' are called out here, going to spend a little more time looking one place above these calls to handled unexpected error rather than this particular throw
There was a problem hiding this comment.
Thought this one through a lot and figured out where the throw needs to happen.
The iterateEmitter will properly throw if the connection is closed while it's iterating, but if it were to call into this _messages while starting in a closed state, it would never throw again. Therefore I moved the check for an already closed socket to the top of _messages
src/agent/at-driver.js
Outdated
| @@ -100,6 +109,7 @@ export class ATDriver { | |||
| * @returns {AsyncGenerator<string>} | |||
| */ | |||
| async *speeches() { | |||
| if (this.hasClosed) throw new Error('AT-Driver connection unexpectedly closed'); | |||
There was a problem hiding this comment.
This check is now redundant since the following line invokes the _messages method.
src/agent/at-driver.js
Outdated
| @@ -70,6 +78,7 @@ export class ATDriver { | |||
| const id = this._nextId++; | |||
| const rawMessage = JSON.stringify({ id, ...command }); | |||
| this.log(AgentMessage.AT_DRIVER_COMMS, { direction: 'outbound', message: rawMessage }); | |||
| if (this.hasClosed) throw new Error('AT-Driver connection unexpectedly closed'); | |||
There was a problem hiding this comment.
The call to WebSocket#send on the subsequent statement will fail in this case. I'd prefer to remove this check and instead await a Promise built from that callback. Reason being: the "ws" library might signal failure in the "send" operation for other reasons that we're currently ignoring.
|
Thanks, @gnarf! |
If the AT-Driver crashes/disconnects the websocket mid-run, this should now throw in an appropriate area to immediately terminate with a message of some sort.