electric
electric copied to clipboard
fix(client): asyncEventEmitter to not silence unhandled exceptions raised in event handlers
Currently if there is an unhandled exception in an event handler of asyncEventEmitter they are silenced and don't make it to a global error handler or console. This changes it to re-throw them async using queueMicrotask
so that they happen outside of the promise awaited by allSettled
.
@msfstef
it would be better to allow listeners to specify error handling as well
I think a listener should handle it's own errors in the usual way, they can wrap their whole handler in a try
if needed.
@samwillis definitely possible to leave it like that, although making the listener accept an error handler and wrapping it internally feels cleaner and forces the subscriber to handle errors (i.e. not a choice)
@samwillis turns out the underlying issue was one resolved by https://github.com/electric-sql/electric/pull/1251, but I still changed the processing
logic in the event emitter to expose a way to be able to wait for any currently running async listeners.
I've made the client shutdown async in order to make sure that things finish running before shutting it down completely - it's still not as clean as I'd like but I think it should resolve the test issues for now.
Rebasing also exposed the issue reported by https://github.com/electric-sql/electric/issues/1283 so I fixed that as well in this PR as it was related.
@msfstef Looks good to me, at least for now. I agree we need to re-arch some of this.
There is a failing test, not sure if that's the flaky one or a new one?
@samwillis the e2e was not flaky, the async event emitter change revealed a minor regression we had during the PG in the client integration where bytearrays were not being deserialized to pure Uint8Array
s - not a significant issue, I doubt any real users would encounter it, but it was making better-sqlite3 throw for the empty bytearray edge case.