-
Notifications
You must be signed in to change notification settings - Fork 165
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
RJS-2784 Fix callback crashes when reloading with React Native #7963
base: master
Are you sure you want to change the base?
Conversation
static constexpr bool cancel_subscription_notifications = false; | ||
shutdown_and_wait(cancel_subscription_notifications); |
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.
Shouldn't this be sending OperationCancelled to any subscription change notifications?
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.
my reasoning was to guard against sub.get_state_change_notification().get_async()
use cases.
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.
Yes, that needs to invoke the completion handler. Every Future must always be fulfilled.
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.
but the completion handler may be gone and crash (pretty much the issue with every handler/callback).
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.
The JS SDK needs to either ensure that App is destroyed before the callback handlers or take responsibility for discarding OperationCancelled calls. The Promise being destroyed without being fulfilled will invoke the completion handler with a BrokenPromise error if we don't explicitly fulfill it.
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 see. I will revert this specific change then cc @gagik
Pull Request Test Coverage Report for Build daniel.tabacaru_887Details
💛 - Coveralls |
ac499d0
to
d3fa720
Compare
e347766
to
d3fa720
Compare
What, How & Why?
See realm/realm-js#6579 and realm/realm-js#6824
When hot reloading with React Native and using sync session connection change callbacks, the app crashes. This is because the callbacks are not cleared and get called. This PR unregisters all callbacks set through the SyncSession when the SyncManager is destroyed as a result of the App being destroyed.
Fixes realm/realm-js#6579.
☑️ ToDos
bindgen/spec.yml
, if public C++ API changed