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

Warn/throw on unexpected attach addon socket state #4208

Merged
merged 1 commit into from
Oct 17, 2022
Merged

Warn/throw on unexpected attach addon socket state #4208

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 18 additions & 4 deletions addons/xterm-addon-attach/src/AttachAddon.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,16 +47,14 @@ export class AttachAddon implements ITerminalAddon {
}

private _sendData(data: string): void {
// TODO: do something better than just swallowing
// the data if the socket is not in a working condition
if (this._socket.readyState !== 1) {
if (!this._checkOpenSocket()) {
return;
}
this._socket.send(data);
}

private _sendBinary(data: string): void {
if (this._socket.readyState !== 1) {
if (!this._checkOpenSocket()) {
return;
}
const buffer = new Uint8Array(data.length);
Expand All @@ -65,6 +63,22 @@ export class AttachAddon implements ITerminalAddon {
}
this._socket.send(buffer);
}

private _checkOpenSocket(): boolean {
switch (this._socket.readyState) {
case WebSocket.OPEN:
return true;
case WebSocket.CONNECTING:
throw new Error('Attach addon was loaded before socket was open');
case WebSocket.CLOSING:
console.warn('Attach addon socket is closing');
return false;
Copy link
Member

Choose a reason for hiding this comment

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

Why not throwing here as well? Would this create race conditions during app shutdown?

Copy link
Member Author

Choose a reason for hiding this comment

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

Was worried about breaking something as we dispose on 'close', not closing. Probably doesn't matter 🤷

case WebSocket.CLOSED:
throw new Error('Attach addon socket is closed');
default:
throw new Error('Unexpected socket state');
}
}
}

function addSocketListener<K extends keyof WebSocketEventMap>(socket: WebSocket, type: K, handler: (this: WebSocket, ev: WebSocketEventMap[K]) => any): IDisposable {
Expand Down