-
Notifications
You must be signed in to change notification settings - Fork 130
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
fix(client): asyncEventEmitter to not silence unhandled exceptions raised in event handlers #1247
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
2aaa322
to
8d40d05
Compare
private async _applySubscriptionData( | ||
changes: InitialDataChange[], | ||
lsn: LSN, | ||
additionalStmts: Statement[] = [], | ||
subscriptionId?: string | ||
) { | ||
): Promise<boolean> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left one comment wrt queueMicrotask()
function, for the rest LGTM.
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've opened a small PR to revert this to the original error throwing: #1312
Minor correction to the fix implemented in #1247 Reintroducing the `queueMicrotask` throwing approach to ensure errors are definitely not suppressed (e.g. they need to be thrown independently of all listener promises `settling`/fulfilling) and ideally if more than one of them occur, all of them are exposed to the global error handler. See discussion here #1247 (comment) The `queueMicrotask` API seems to be widely supported by browsers and runtimes so I don't think there's a need to worry. The alternative is also to use `setTimeout` which is also widely supported, but that might cause the errors to be thrown significantly later than when they were emitted.
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 byallSettled
.