Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

danieltabacaru
Copy link
Collaborator

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

  • 📝 Changelog update
  • 🚦 Tests (or not relevant)
  • C-API, if public C++ API changed
  • bindgen/spec.yml, if public C++ API changed

@cla-bot cla-bot bot added the cla: yes label Aug 7, 2024
@danieltabacaru danieltabacaru changed the title Fix callback crashes when reloading with React Native RJS-2784 Fix callback crashes when reloading with React Native Aug 7, 2024
Comment on lines 416 to 417
static constexpr bool cancel_subscription_notifications = false;
shutdown_and_wait(cancel_subscription_notifications);
Copy link
Member

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?

Copy link
Collaborator Author

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.

Copy link
Member

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.

Copy link
Collaborator Author

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

Copy link
Member

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.

Copy link
Collaborator Author

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

Copy link

coveralls-official bot commented Aug 7, 2024

Pull Request Test Coverage Report for Build daniel.tabacaru_887

Details

  • 35 of 37 (94.59%) changed or added relevant lines in 2 files are covered.
  • 294 unchanged lines in 9 files lost coverage.
  • Overall coverage increased (+0.02%) to 91.108%

Changes Missing Coverage Covered Lines Changed/Added Lines %
test/object-store/sync/session/connection_change_notifications.cpp 14 16 87.5%
Files with Coverage Reduction New Missed Lines %
src/realm/sync/noinst/client_impl_base.cpp 1 82.88%
test/fuzz_tester.hpp 1 57.73%
src/realm/sync/changeset.cpp 2 39.15%
test/object-store/util/test_file.cpp 2 86.47%
src/realm/table.cpp 3 90.59%
src/realm/sync/noinst/protocol_codec.cpp 8 82.51%
src/realm/sync/noinst/server/server.cpp 19 74.02%
test/test_sync.cpp 114 93.03%
src/realm/query_conditions.hpp 144 69.04%
Totals Coverage Status
Change from base Build 2554: 0.02%
Covered Lines: 217072
Relevant Lines: 238259

💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

React Native Android application crashing when using syncSession.addConnectionNotification
2 participants