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.

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

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

Closed
loucadufault opened this issue Aug 12, 2024 · 1 comment
Closed

Comments

@loucadufault
Copy link
Contributor

loucadufault commented Aug 12, 2024

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:

public write(data: number[]): void {
this.reportQueue
.add(async () => {
await this.device.sendReport(data[0], new Uint8Array(data.slice(1)))
})
.catch((err) => {
this.emit('error', err)
})
}

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.

@loucadufault loucadufault changed the title WebHID: methods which call write (sendReport) to the device should be async WebHID: methods which write (sendReport) to the device should be async Aug 12, 2024
@loucadufault loucadufault changed the title WebHID: methods which write (sendReport) to the device should be async WebHID: methods which write (sendReport) to the device should be async Aug 12, 2024
@nytamin
Copy link
Member

nytamin commented Aug 26, 2024

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

  • Add a flush(): Promise<void> method to wait for any lingering writes.
  • Also: Once close() is called, promise to never emit any errors anymore.

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants