Fix connection callback crashes when reloading with React Native #7936
Conversation
gagik
force-pushed
the
gagik/fix-connection-callback-crashes
branch
from
bdee157
to
953e657
Compare
| @@ -224,8 +229,10 @@ void SyncSession::do_become_inactive(util::CheckedUniqueLock lock, Status status | |||
|
|
|||
| // Send notifications after releasing the lock to prevent deadlocks in the callback. | |||
| if (old_state != new_state) { | |||
| m_connection_change_notifier.invoke_callbacks(old_state, connection_state()); | |||
| // m_connection_change_notifier.invoke_callbacks(old_state, connection_state()); | |||
There was a problem hiding this comment.
So this is the problem here... I am not sure if this is some race condition of sort but even just commenting this out makes the crashes at least 50% less rare on hot reload (I realize this is useful in all other scenarios so it shouldn't stay like this, just not sure how to tackle that).
| } | ||
| m_connection_change_notifier.clear_callbacks(); |
There was a problem hiding this comment.
This basically fixes these crashes completely
| @@ -397,6 +397,25 @@ void SyncManager::close_all_sessions() | |||
| get_sync_client().wait_for_session_terminations(); | |||
| } | |||
|
|
|||
|
|
|||
| void SyncManager::clear_connection_change_callbacks() | |||
There was a problem hiding this comment.
One idea I had was to expose this through the app and manually run that the way we do clearAllAppCaches in this code it isn't yet exposed.
There was a problem hiding this comment.
have you tried using this (or close_all_sessions())? I am curious if React Native waits for it to complete.
There was a problem hiding this comment.
I believe I have but will re-check now
There was a problem hiding this comment.
@danieltabacaru I have actually been running close_all_sessions as App::close_all_sync_sessions calls this and eventually this hits the become_dying > become_inactive > do_become_inactive function I have been modifying. Without the changes in the PR I don't think the listeners are actually removed, at least not on time
It actually may be awaiting this close_all_sync_sessions call as it gets made in the start of loading the new state after reload rather than the end of the old one so if the function is blocking correctly I believe it should await.
You can see the JS side of things here: realm/realm-js#6824
There was a problem hiding this comment.
I think I finally understand what's going on. When the app reloads, the connection callback of an old SyncSession is invoked, but the callback is long gone (and hence the crash). Does this sound right?
There was a problem hiding this comment.
@danieltabacaru Yes exactly
|
|
||
| m_callbacks.clear(); | ||
| m_callback_count = 0; | ||
| m_callback_index = npos; // Reset callback index since all callbacks are removed |
There was a problem hiding this comment.
There may be a better way to do this
danieltabacaru
force-pushed
the
gagik/fix-connection-callback-crashes
branch
from
953e657
to
819bb98
Compare
Pull Request Test Coverage Report for Build github_pull_request_301264Details
💛 - Coveralls |
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 attempts to fix it.
My solution has been to add some form of
clear_all_callbacksand run in either always when abruptly stopping sync sessions or in a specific newly exposed function.There may be better alternatives but hopefully this will showcase the issue better. The changes do make the crashes more rare but if made quickly enough, the app can still crash. This other crash has no stack trace so it's much harder to debug but may be related to some mistake being made here.
☑️ ToDos
bindgen/spec.yml, if public C++ API changed