Fix exception caused by a race condition in the database module on iOS

[jeanregisser]

Summary

I came across a race condition while working on our app: https://github.com/celo-org/celo-monorepo/tree/master/packages/mobile

The exception raised is createRepo called for Repo that already exists. in FRepoManager.m

It happens when using a transaction and db listener at the same time after initializing firebase.
https://github.com/celo-org/celo-monorepo/blob/833c2d3d2341185dc141aec901c9b2828e776de6/packages/mobile/src/firebase/firebase.ts#L21

Exception stacktrace

Exception 'createRepo called for Repo that already exists.' was thrown while invoking on on target RNFirebaseDatabase with params (
    "[DEFAULT]",
    "https://celo-org-mobile-int.firebaseio.com/",
        {
        appName = "[DEFAULT]";
        eventType = value;
        hasCancellationCallback = 0;
        key = "$https://celo-org-mobile-int.firebaseio.com/$/.info/serverTimeOffset${}";
        modifiers =         (
        );
        path = ".info/serverTimeOffset";
        registration =         {
            eventRegistrationKey = "$https://celo-org-mobile-int.firebaseio.com/$/.info/serverTimeOffset${}$0$value";
            key = "$https://celo-org-mobile-int.firebaseio.com/$/.info/serverTimeOffset${}";
            registrationCancellationKey = "$https://celo-org-mobile-int.firebaseio.com/$/.info/serverTimeOffset${}$0$value$cancelled";
        };
    }
)
callstack: (
  0   CoreFoundation                      0x0000000108c058db __exceptionPreprocess + 331
  1   libobjc.A.dylib                     0x0000000107d6aac5 objc_exception_throw + 48
  2   CoreFoundation                      0x0000000108c05735 +[NSException raise:format:] + 197
  3   celo                                0x0000000101d0782e +[FRepoManager createRepo:config:database:] + 878
  4   celo                                0x0000000101cba88c -[FIRDatabase ensureRepo] + 188
  5   celo                                0x0000000101cb9c78 -[FIRDatabase reference] + 40
  6   celo                                0x0000000101e5c740 -[RNFirebaseDatabaseReference buildQueryAtPathWithModifiers:modifiers:] + 208
  7   celo                                0x0000000101e5b129 -[RNFirebaseDatabaseReference initWithPathAndModifiers:appDisplayName:dbURL:key:refPath:modifiers:] + 521
  8   celo                                0x0000000101e59979 -[RNFirebaseDatabase getCachedInternalReferenceForApp:dbURL:props:] + 393
  9   celo                                0x0000000101e58f4b -[RNFirebaseDatabase on:dbURL:props:] + 155
  10  CoreFoundation                      0x0000000108c0c6ac __invoking___ + 140
  11  CoreFoundation                      0x0000000108c09c25 -[NSInvocation invoke] + 325
  12  CoreFoundation                      0x0000000108c0a076 -[NSInvocation invokeWithTarget:] + 54
  13  React                               0x0000000106f7283a -[RCTModuleMethod invokeWithBridge:module:arguments:] + 2810
  14  React                               0x0000000106f7f026 _ZN8facebook5reactL11invokeInnerEP9RCTBridgeP13RCTModuleDatajRKN5folly7dynamicE + 790
  15  React                               0x0000000106f7eb33 _ZZN8facebook5react15RCTNativeModule6invokeEjON5folly7dynamicEiENK3$_0clEv + 131
  16  React                               0x0000000106f7eaa9 ___ZN8facebook5react15RCTNativeModule6invokeEjON5folly7dynamicEi_block_invoke + 25
  17  libdispatch.dylib                   0x000000010a34fd7f _dispatch_call_block_and_release + 12
  18  libdispatch.dylib                   0x000000010a350db5 _dispatch_client_callout + 8
  19  libdispatch.dylib                   0x000000010a358225 _dispatch_lane_serial_drain + 778
  20  libdispatch.dylib                   0x000000010a358e9c _dispatch_lane_invoke + 425
  21  libdispatch.dylib                   0x000000010a362ea3 _dispatch_workloop_worker_thread + 733
  22  libsystem_pthread.dylib             0x000000010a739611 _pthread_wqthread + 421
  23  libsystem_pthread.dylib             0x000000010a7393fd start_wqthread + 13

It looks like we have to get the firebase database reference from the same
thread/queue otherwise we have a race condition leading to that exception.

Here are screenshots showing the race condition captured while debugging the app.
You can see the 2 different code paths calling createRepo: from 2 different threads racing to cause the exception.

first the transaction

second the db listener

This might affect v6 but we haven't upgraded yet.

Checklist

• Supports Android
• Supports iOS
• e2e tests added or updated in packages/**/e2e
• Flow types updated
• Typescript types updated

Test Plan

Ran our app without this patch -> the exception is raised.
Ran our app with this patch -> no exception is raised and the app behaves correctly.

Release Plan

[IOS] [BUGFIX] [DATABASE] - Fix race condition causing exception 'createRepo called for Repo that already exists.' to be raised when using transaction and db listener at roughly the same time.

[mikehardy]

Fascinating - this one is above my pay grade in Objective-C land but it appears to affect v6 as well - @Salakar / @Ehesp what think you?

https://github.com/invertase/react-native-firebase/blob/master/packages/database/ios/RNFBDatabase/RNFBDatabaseTransactionModule.m#L68

[valentinjob]

we are having the same problem. Are there any way to merge it?

[mikehardy]

@valentinjob since release dates are unpredictable for open-source (well, for closed source too, but with open source you at least have the source) I strongly recommend you integrate the patch-package tool, and for situations where there is a known fix for a problem you have, integrate the patch in your code-base. You'll get a nice notice when later releases include the fix, but you'll be working without problem in the meanwhile.

I can't remember I time in the last year where I was not carrying around at least one patch via patch-package but they are always different as they are integrated upstream and released.

[codecov]

Codecov Report

Merging #2734 into v5.x.x will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           v5.x.x    #2734   +/-   ##
=======================================
  Coverage   76.02%   76.02%           
=======================================
  Files          61       61           
  Lines        1576     1576           
=======================================
  Hits         1198     1198           
  Misses        378      378

[mikehardy]

Hey there - are either of you two using this in the wild? To add some confidence that we know it's working?

I have the v5.x.x branch in a very clean state right now, with a reliable test setup locally to get around the current v5 CI failures, and could easily merge a couple more patches in

[jeanregisser]

@mikehardy I've been using the patch in the wild for about 5 weeks now. Seems to work fine.

[mikehardy]

Thanks for the feedback @jeanregisser - I'll queue this for the next v5 release then

[Salakar]

LGTM

[Ehesp]

cc @russellwheatley one to update on v6

[russellwheatley]

@Ehesp I think v6 doesn't need updating as the equivalent reference has already been taken out of the async dispatch: https://github.com/invertase/react-native-firebase/blob/master/packages/database/ios/RNFBDatabase/RNFBDatabaseTransactionModule.m#L68