SuperFlyTV / xkeys

A Node.js module to interact with the X-keys panels.
MIT License
33 stars 12 forks source link

WebHID: methods which write (`sendReport`) to the device should be async #106

Open loucadufault opened 1 month ago

loucadufault commented 1 month ago

The implementation of the generic write() method which is called by the various XKeys methods (e.g. setting backlights) is asynchronous in nature, yet resolves synchronously before the promise returned by the underlying call to sendReport() has resolved:

https://github.com/SuperFlyTV/xkeys/blob/e9d925e22d184775e252c1991a1105bfedc14311/packages/webhid/src/web-hid-wrapper.ts#L24-L32

Promise rejections are handled by emitting them as error events.

This forces usage of the API to always have an error listener registered for the duration of the program in order for the code to be safe; if the listener is removed at any point, it is possible that a pending promise rejects and emits an error event which would be unhandled.

This makes it near impossible to clean up an XKeys object by calling methods on it such as setAllBacklights() before un-registering the listeners such that it can be GC'd:

// setup
const xKeys = setupXKeysPanel(device)
const errorListener = (e) => console.error(e)
xKeys.on('error', errorListener)

// ... later, after an abnormal event
// cleanup
xKeys.setAllBacklights(false)
xKeys.removeListener('error', errorListener)
// the promise indirectly created from the call to `setAllBacklights` resolves at some point after the synchronous code, when the error listener has already been un-registered

A workaround is to keep the listener registered for an arbitrary amount of time to handle any pending promises:

// cleanup
xKeys.setAllBacklights(false)
setTimeout(() => xKeys.removeListener('error', errorListener), 5000)

A proper solution would be to allow checking that all sendReport promises have settled before cleaning up the device, which would entail making the API methods async.

nytamin commented 2 weeks ago

After some consideration, I propose these changes to address this:

With these changes, you should be able to acheive a proper cleanup like so:

const xKeys = setupXKeysPanel()
const errorListener = (e) => console.error(e)
xKeys.on('error', errorListener)

xKeys.setAllBacklights(false)

// cleanup:
await xKeys.flush() // waits for all writes to finish
await xKeys.close() // close the device.
xKeys.off('error', errorListener)