Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add some thrown errors if the at driver has closed #57

Merged
merged 4 commits into from
Jul 11, 2024
Merged

Add some thrown errors if the at driver has closed #57

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
476aab3
Add some thrown errors if the at driver has closed
gnarf Jun 27, 2024
7abe477
throw trying to collect _messages after close has happened
gnarf Jul 3, 2024
21d1b36
move more hasClosed checks to before the _messages
gnarf Jul 3, 2024
8e405b2
Review feedback - thanks mike!
gnarf Jul 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion src/agent/at-driver.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,14 @@ export class ATDriver {
}
)
);
this.closed = new Promise(resolve => socket.once('close', () => resolve()));
this.hasClosed = false;
this.closed = new Promise(resolve =>
socket.once('close', () => {
this.hasClosed = true;
this.log(AgentMessage.AT_DRIVER_COMMS, { direction: 'closed' });
resolve();
})
);
this._nextId = 0;
}

Expand All @@ -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');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

yield JSON.parse(message);
}
}
Expand All @@ -70,8 +78,10 @@ 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');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

this.socket.send(rawMessage);
for await (const message of this._messages()) {
if (this.hasClosed) throw new Error('AT-Driver connection unexpectedly closed');
if (message.id === id) {
if (message.error) {
throw new Error(message.error);
Expand Down Expand Up @@ -101,6 +111,7 @@ export class ATDriver {
*/
async *speeches() {
for await (const message of this._messages()) {
if (this.hasClosed) throw new Error('AT-Driver connection unexpectedly closed');
if (message.method === 'interaction.capturedOutput') {
yield message.params.data;
}
Expand Down
Loading