fix(hci): Fix possible deadlock when connecting

Author: Marco Crespi

lib/bindings/hci/misc/Hci.js

@@ -42,11 +42,20 @@ export declare abstract class Peripheral {
     get state(): PeripheralState;
     constructor(adapter: Adapter, uuid: string, addressType: AddressType, address: string, advertisement?: any, rssi?: number);
     /**
-     * Connect to this peripheral. Does nothing if already connected.
+     * Connect to this peripheral. Throws an error when connecting fails.
      */
     abstract connect(): Promise<void>;
     /**
-     * Disconnect from this peripheral. Does nothing if not connected.
+     * Disconnect from this peripheral. Does nothing if not connected. This method **never** throws an error.
+     * When connecting to a peripheral you should always wrap your calls in try-catch and call this method at the end.
+     * ```
+     * try {
+     *   peripheral.connect()
+     * } catch (err) {
+     *   ...
+     * } finally {
+     *   peripheral.disconnect();
+     * }```
      */
     abstract disconnect(): Promise<void>;
     /**

lib/bindings/hci/misc/Hci.js.map

@@ -317,37 +317,38 @@ export class Hci extends EventEmitter {
 		const timeoutError = new Error(`HCI command timed out`);
 
 		return new Promise<Buffer | void>((resolve, reject) => {
-			let isDone = false;
+			let timeout: NodeJS.Timeout;
+			let onComplete: (status: number, responseData?: Buffer) => void;
 
-			const resolveHandler = (response?: Buffer) => {
-				if (isDone) {
-					return;
+			const cleanup = () => {
+				if (statusOnly) {
+					this.off('cmdStatus', onComplete);
+				} else {
+					this.off('cmdComplete', onComplete);
+				}
+
+				if (timeout) {
+					clearTimeout(timeout);
+					timeout = null;
 				}
-				isDone = true;
 
 				this.currentCmd = null;
 				if (release) {
 					release();
 				}
+			};
 
+			const resolveHandler = (response?: Buffer) => {
+				cleanup();
 				resolve(response);
 			};
 
 			const rejectHandler = (error?: any) => {
-				if (isDone) {
-					return;
-				}
-				isDone = true;
-
-				this.currentCmd = null;
-				if (release) {
-					release();
-				}
-
+				cleanup();
 				reject(error);
 			};
 
-			const onComplete = (status: number, responseData?: Buffer) => {
+			onComplete = (status: number, responseData?: Buffer) => {
 				if (status !== 0) {
 					const errStatus = `${STATUS_MAPPER[status]} (0x${status.toString(16).padStart(2, '0')})`;
 					const err = new Error(`HCI Command ${this.currentCmd.cmd} failed: ${errStatus}`);
@@ -366,10 +367,10 @@ export class Hci extends EventEmitter {
 				this.once('cmdComplete', onComplete);
 			}
 
+			timeout = setTimeout(() => rejectHandler(timeoutError), this.cmdTimeout);
+
 			// console.log('->', 'hci', data);
 			this.socket.write(data);
-
-			setTimeout(() => rejectHandler(timeoutError), this.cmdTimeout);
 		});
 	}
 
@@ -579,8 +580,6 @@ export class Hci extends EventEmitter {
 		cmd.writeUInt16LE(0x0004, 25); // min ce length
 		cmd.writeUInt16LE(0x0006, 27); // max ce length
 
-		const origScope = new Error();
-
 		const release = await this.mutex.acquire();
 
 		try {
@@ -590,7 +589,32 @@ export class Hci extends EventEmitter {
 		}
 
 		return new Promise<number>((resolve, reject) => {
-			const onComplete: LeConnCompleteListener = async (status, handle, role, _addressType, _address) => {
+			const origScope = new Error();
+			let timeout: NodeJS.Timeout;
+			let onComplete: LeConnCompleteListener;
+
+			const cleanup = () => {
+				this.off('leConnComplete', onComplete);
+
+				if (timeout) {
+					clearTimeout(timeout);
+					timeout = null;
+				}
+
+				release();
+			};
+
+			const resolveHandler = (handle: number) => {
+				cleanup();
+				resolve(handle);
+			};
+
+			const rejectHandler = (error?: any) => {
+				cleanup();
+				reject(error);
+			};
+
+			onComplete = async (status, handle, role, _addressType, _address) => {
 				if (_address !== address || _addressType !== addressType) {
 					return;
 				}
@@ -602,30 +626,27 @@ export class Hci extends EventEmitter {
 					const err = new Error(`LE conn failed: ${errStatus}`);
 					err.stack = err.stack.split('\n').slice(0, 2).join('\n') + '\n' + origScope.stack;
 
-					release();
-					reject(err);
+					rejectHandler(err);
 					return;
 				}
 
 				if (role !== 0) {
 					const err = new Error(`Could not aquire le connection as master role`);
 					err.stack = err.stack.split('\n').slice(0, 2).join('\n') + '\n' + origScope.stack;
 
-					release();
-					reject(err);
+					rejectHandler(err);
 					return;
 				}
 
-				release();
-				resolve(handle);
+				resolveHandler(handle);
 			};
 
 			this.on('leConnComplete', onComplete);
 
-			this.sendCommand(cmd, true, true).catch((err) => {
-				release();
-				reject(err);
-			});
+			const timeoutError = new Error(`Creating connection timed out`);
+			timeout = setTimeout(() => rejectHandler(timeoutError), this.cmdTimeout);
+
+			this.sendCommand(cmd, true, true).catch((err) => rejectHandler(err));
 		});
 	}
 

lib/models/Peripheral.d.ts

@@ -69,11 +69,20 @@ export abstract class Peripheral {
 	}
 
 	/**
-	 * Connect to this peripheral. Does nothing if already connected.
+	 * Connect to this peripheral. Throws an error when connecting fails.
 	 */
 	public abstract connect(): Promise<void>;
 	/**
-	 * Disconnect from this peripheral. Does nothing if not connected.
+	 * Disconnect from this peripheral. Does nothing if not connected. This method **never** throws an error.
+	 * When connecting to a peripheral you should always wrap your calls in try-catch and call this method at the end.
+	 * ```
+	 * try {
+	 *   peripheral.connect()
+	 * } catch (err) {
+	 *   ...
+	 * } finally {
+	 *   peripheral.disconnect();
+	 * }```
 	 */
 	public abstract disconnect(): Promise<void>;
 

lib/models/Peripheral.js.map

@@ -112,9 +112,7 @@ const main = async () => {
 		} finally {
 			console.log('Disconnecting...');
 
-			try {
-				await peripheral.disconnect();
-			} catch {}
+			await peripheral.disconnect();
 
 			console.log('Disconnected');
 		}

src/bindings/hci/misc/Hci.ts

@@ -107,9 +107,7 @@ const main = async () => {
 		} finally {
 			console.log(targetAddress, 'Disconnecting...');
 
-			try {
-				await peripheral.disconnect();
-			} catch {}
+			await peripheral.disconnect();
 
 			console.log(targetAddress, 'Disconnected');
 		}