fix(client): asyncEventEmitter to not silence unhandled exceptions raised in event handlers #1247
Conversation
There was a problem hiding this comment.
I feel like it would be better to allow listeners to specify error handling as well and forward the error handling to them but this is definitely better than suppressing the error entirely!
Could you add a test for this as well? we can mock the global error handler in the test to assert it catches this
I think a listener should handle it's own errors in the usual way, they can wrap their whole handler in a |
|
@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
force-pushed
the
samwillis/fix-asyncEventEmmitter-errors
branch
from
2aaa322
to
8d40d05
Compare
| private async _applySubscriptionData( | ||
| changes: InitialDataChange[], | ||
| lsn: LSN, | ||
| additionalStmts: Statement[] = [], | ||
| subscriptionId?: string | ||
| ) { | ||
| ): Promise<boolean> { |
There was a problem hiding this comment.
really ugly fix but this is a symptom of how we've mixed and matched the architecture here - if _applySubscriptionData fails, it still resolves but fires _handleSubscriptionError. However the error handler resets the state of the subscriptionManager, so the afterApply call in lines 513-516 still fires because _applySubscriptionData resolves without an issue but because the state of the subscription manager has been reset it attempts to resolve promises that are no longer present.
I think satellite probably needs a bit of rearchitecting as we're seeing various issues w.r.t. timing and async.
|
@samwillis turns out the underlying issue was one resolved by #1251, but I still changed the 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 #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 |
| // If a listener throws an error, we re-throw it asynchronously so that the queue can continue | ||
| // to be processed, this ensures that the exception isn't swallowed by allSettled below. | ||
| // It will likely be caught by a global error handler, or be logged to the console. | ||
| queueMicrotask(() => { |
There was a problem hiding this comment.
Is queueMicrotask available in all environments?
The JS docs say:
The queueMicrotask() method, which is exposed on the Window or Worker interface
So they are talking about browser environments.
Looked it up for node and node seems to support it as well.
Not sure about native environments such as react-native?
Perhaps we don't need this queueMicrotask trick since allSettled returns an array of promise statuses (fulfilled or rejected). So we could loop over the processingProm array, aggregate all errors into one, and then throw the aggregated error. Would that work?
There was a problem hiding this comment.
I'm pretty sure queueMicrotask is widely supported, but I agree with your suggestion and I think looking at the settled promises and rethrowing is more idiomatic/clear.
It does change the behaviour slightly though as it will only throw once everything has settled, whereas with queueMicrotask the error would be bubbled up as soon as it happens. @samwillis what's your opinion?
There was a problem hiding this comment.
I've merged the PR to avoid having it linger for too long as I think it's an important one to have in main, but if there's any disagreements on this last change let me know.
| // be suppressed and bugs may go unnoticed | ||
| settledPromises.forEach((rejectedProm) => { | ||
| if (rejectedProm.status === 'rejected') { | ||
| throw rejectedProm.reason |
There was a problem hiding this comment.
@msfstef One question. If there were 2 failing promises, wouldn't this only throw one of them? The throw would be stopping the forEach.
There was a problem hiding this comment.
@davidmartos96 you're absolutely right (and I also just noticed that they'recalled rejectedProm before the check because my initial impl was using a filter clause that TS complained about -_-) - it will interrupt the forEach so this is a bit confusing, but I liked it more than using an explicit loop or throwing the zeroth element of the filtered list.
That being said the previous fix using queueMicrotask would not suppress any errors the way this does... so perhaps either we reintroduce something like that, or bundle all of the errors together in a sort of error aggregator.
There was a problem hiding this comment.
@samwillis you're right about the possibility of the promises never resolving and thus never throwing - I think then we all agree to revert the throwing to the listener execution, only detail is whether we want to use queueMicrotask or perhaps setTimeout if we have compatibility concerns.
From what I'm seeing I doubt we'll have issues with queueMicrotask so we might as well go with that and address any compatibility issues if and when they come up.
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
queueMicrotaskso that they happen outside of the promise awaited byallSettled.