electric icon indicating copy to clipboard operation
electric copied to clipboard

fix(client): asyncEventEmitter to not silence unhandled exceptions raised in event handlers

Open samwillis opened this issue 9 months ago • 2 comments

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.

samwillis avatar May 09 '24 09:05 samwillis

@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 avatar May 09 '24 09:05 samwillis

@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)

msfstef avatar May 09 '24 09:05 msfstef

@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 avatar May 21 '24 14:05 msfstef

@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 avatar May 21 '24 16:05 samwillis

@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 Uint8Arrays - 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.

msfstef avatar May 22 '24 09:05 msfstef