React-native-branch-deep-linking-attribution: Opening Android app fresh from Branch.io link runs branch.subscribe callback function twice.

[jasonmerino]

I'm having an issue on Android only where when I launch our app fresh (not resuming from background) from a Branch.io link the callback I provide to the branch.subscribe function get's called with the same bundle payload twice.

This only happens on Android, not on iOS, and I have verified that on the Android side the onNewIntent method is only called once when opening a Branch.io link while our app is backgrounded and onNewIntent isn't called at all when launching the app fresh from the same Branch.io link.

This leads me to believe that the problem is probably on the JS side. Any help or guidance on this would be very much appreciated.

Thanks!

react-native: 0.36.0
react-native-branch: 0.9.0

[jdee]

@jasonmerino Thanks for the info. This sounds like it might be related to another issue I was just working on: #73. The short answer is there is definitely something wrong in the JS layer, but it requires a little research to address it.

If you can use [email protected], you can try a branch in this repo: resubscription-fix. That is a partial fix, at least, for the issue mentioned above. Trying it might at least produce some information, even if it doesn't completely fix your problem.

[jasonmerino]

@jdee Thanks for the speedy response. :)

Upgrading to RN 0.40.0 is not on our roadmap right now as it will take a while since our app is quite large. Is there any hope of applying this patch to a 0.9.1 version of this repo?

[jdee]

@jasonmerino There's always hope.

I'm more immediately concerned about understanding your problem. I suspect you're running afoul of the same caching issue, but I don't know that, or why it doesn't happen on iOS. If you can't use 0.40.0 and are feeling adventurous, you can copy this file (src/index.js from the resubscription-fix branch) to node_modules/react-native-branch/src, restart the React packager and try it out. There were no source changes there in 1.0.0, so you should be able to drop it in.

Also, can I ask what version of Android you're using? It may have an impact, but mainly I'm trying to get an Android test environment going and want to make sure I'm looking at what you're looking at.

Thanks.

[jasonmerino]

@jdee I have applied the src/index.js file to our project and restarted the packager and rebuilt the application. The same problem seems to be still present.

Emulator Android Version: 6.0
Device Android Version: 6.0.1

[jdee]

@jasonmerino Thanks for the info. How urgent is this problem for you? Would it be sufficient if we did a release 0.9.1 next week?

Thanks.

[jasonmerino]

@jdee next week would definitely be acceptable. I'm not happy about my setTimeout workaround, but it will get the job done for now. :)

[jdee]

@jasonmerino I now have a working Android setup. I'm able to deploy a test app to a device or an emulator, both running Marshmallow, open App Links and be called back in JS. I've tried working with the development react-native server and also bundling the JS into the app. If I kill the app on the device or emulator and open an App Link to launch the app, the JS callback is never invoked more than once. I'm working with react-native-branch 1.0, but there were no changes to the JS or native Android code between 0.9 and 1.0. The only bug I'm aware of is that if you unsubscribe and resubscribe, you will receive the last link again after you resubscribe. That is fixed in 0.9.1. So in short, I'm not able to reproduce your problem.

Can you share any of your code to show how you're using the SDK? I wonder if anything other SDK calls could be interfering in some way.

Also, there are more intent receivers involved than just the activity that receives the link. In particular, there is a call here, around line 82 of node_modules/react-native-branch/android/src/main/java/io/branch/rnbranch/RNBranchModule.java. Can you confirm whether that is being called more than once? If that's not firing more than once, it has to be in the JS. But it's strange that this doesn't occur for you on iOS. That suggests it could be a matter of timing between the two native layers. But it's also strange that I can't produce the problem on Android, so I'm obviously not looking at what you're looking at.

Finally, there is a branch here called release-0.9.1, which is a preview of what will be released as 0.9.1. I don't think that will have any impact on your problem, but you can always try it out.

[timursadykhov]

Hi @jdee I'm seeing this issue as well and could really use some help. Here is the information I can provide:

react-native: 0.42.0
react-native-branch: 1.1.1

My deep linking integration works fine on iOS but on android it opens the route twice.

I did some debugging and found that _onInitSessionResult is called twice. The first time it's called from the INIT_SESSION_SUCCESS listener nativeEventEmitter.addListener(INIT_SESSION_SUCCESS, this._onInitSessionResult) and the second time it's called when redeemInitSessionResult returns. Seems like the code wants this to happen so I'm not really understanding what's going wrong. Both times the result variable has the same value.

Let me know if I can provide any more information.

[jdee]

@timursadykhov Interesting. Thanks for investigating. I couldn't reproduce it when I last looked. I'll check again and see if I can reproduce it.

Incidentally, there is now a webview_example in the examples folder in master. That uses 2.0.0, but there are not many SDK differences between 1.1 and 2.0 yet, mainly just changes to the way things are built. The main difference on Android is that 2.0 includes a jar for the Branch SDK. This illustrates link routing, among other things. It would be interesting to know if you can reproduce the same problem with that app. You could test by setting up a new app in the Branch Dashboard with that package identifier (com.webview_example) and then set up intent-filters in the AndroidManifest.xml and modify the Branch key. Then just yarn or npm install and run. You can share links from the app, open and see if the callback fires twice.

[timursadykhov]

@jdee okay, i'll try that out. in the meantime I can tell you that commenting out the line if (result) this._onInitSessionResult(result) fixes my issue and results in everything working as expected. For what scenario is that line necessary?

[jdee]

@timursadykhov I think you've done a better job of analyzing this code than I did. It looks to me like that call is unnecessary. The underlying issue is a race between initializing the Branch session in the native layer and loading the JS. The link that launched the app may already have been received by the native layer before the JS loads. That's the idea behind that call. However, there's also a cache used by the subscribe method.

I don't reproduce this problem with the webview_example, but that may be because it's small. If your app is large, and the JS doesn't finish loading until after the initial link has been received by the native layer, it looks like it will fire twice as is. And maybe it just loads faster on iOS, so the JS wins the race, and the callback only fires once.

I'll get this fixed and released. Thanks for the help. Branch would be happy to reward you with swag for your assistance. You can contact [email protected] for details.

[timursadykhov]

@jdee glad to help! :)

Your diagnosis sounds right to me. I was only seeing the issue about 90% of the time.

[jdee]

@timursadykhov There is now a branch called maintenance-1.1 that contains version 1.1.2 with this fix. Would you mind trying it out to see if it fixes your problem? It worked in the webview_example after adding delays here and there to simulate loading a large RN app.

[timursadykhov]

@jdee sure thing, I'll do that tonight and get back to you

[jdee]

@timursadykhov If you're only calling subscribe once in your app, that branch will probably work. I think there's more work to do to support multiple subscribers properly. In short, the issue is fairly thorny, and there will probably be further iterations. I'd be interested to know how that branch works for you, but expect further changes before this is released.

[timursadykhov]

@jdee Using the branch now and it's passed all my tests. We are indeed subscribing only once.

Since we need this fix I'm planning to continue using the maintenance-1.1 branch ... unless you have a different suggestion?

[timursadykhov]

@jdee Actually I spoke too soon :)

The new RNBranch.redeemInitSessionResult() in subscribe is still causing a second call to my callback. Maybe the native cache isn't being cleared like expected?

[timursadykhov]

@jdee okay i did some investigating, here's a breakdown of what's happening.

The order of events is as follows:

1. Native - sendRNEvent with RN_INIT_SESSION_EVENT is called from forwardInitSessionFinishedEventToReactNative
   
   
   
   • the event is immediately successfully emitted (i see Catalyst instance active in the console)
   
   
2. Javascript - RNBranch.redeemInitSessionResult is called in the subscribe method (time since launch here is on average 580ms)
3. Native - redeemInitSessionResult finishes and clears the cache
4. Javascript - _onInitSessionResult is called from our INIT_SESSION_SUCCESS listener in response to the event from 1
   
   
   
   • it still has a result since the event actually happened before we clear the cache in the redeemInitSessionResult call in 3
   
   
5. Javascript - the promise from 2 RNBranch.redeemInitSessionResult returns and calls our callback a second time

I don't really understand the code well enough to suggest any solutions though :)

My only thought is that maybe sendRNEvent should clear the cache itself in the RN_INIT_SESSION_EVENT since we know it's successful before redeemInitSessionResult is even called.

[jdee]

@timursadykhov Thanks for the testing and analysis. It's very helpful.

There are some other requirements that complicate matters:

• Multiple subscription is supported. You can call branch.subscribe from multiple places in the JS and get back the same link events everywhere it's called. This includes any initial link cached by the native or JS layer.
• Unsubscription/resubscription is supported. You can call the result of branch.subscribe to cancel your subscription, then call branch.subscribe again. You won't get the same link twice. In particular, after unsubscribing, when you resubscribe, you won't get the initial link again. This is the reason for the INIT_SESSION_TTL in index.js.
• The only way I'm aware of to call the native layer from JS is asynchronously, via promises. There doesn't seem to be any support for synchronous calls, so things like getFirstReferringParams and createBranchUniversalObject, which are fundamentally synchronous, always have to be asynchronous. This is a big part of the problem. The way the JS layer is designed is fine, except that redeemInitSessionResult should be synchronous. Instead there's a race between the promise to that method and the call to the same method in subscribe. If these were both synchronous, I don't think there would be a problem.

The idea behind the current implementation, I believe, is that calls into the native layer are minimized. The native layer caches each response but only until it is retrieved by RNBranch.redeemInitSessionResult. Once this is called, it's up to the JS layer to cache. The JS layer handles the fanout to multiple listeners. Hence it maintains a cache at launch, in case of races and multiple subscriptions. After a few seconds, it voids the cache and stops caching.

Obviously this is a little complicated. I'd like to simplify it to make it simpler to understand and maintain. This will make greater use of the native layer and make the calls through a little more transparent. We can evaluate performance implications once it's working properly. If there is extra overhead to call the native layer, it will be at least partly offset by improved performance of operations performed by the native layer instead of JS.

The native layer already fans out notifications to multiple listeners by emitting an event. and the emitted result is never modified by the JS layer. The callback to subscribe can just be passed to addEventListener directly, and when unsubscribing, removeEventListener can be called. The _listeners can be removed from the Branch class in index.js.

The native cache can be changed so that rather than being cleared immediately on retrieval, it has the TTL currently used by the JS layer. Then the cache can be completely removed from the JS layer. When a cached value is needed, it can be retrieved asynchronously from the native layer and returned to the subscribe listener when that promise is resolved. There should then be no risk of getting the cached value twice.

I'm going to head down this path and try to get you a fix by the end of the day. If you can confirm the fix, I can release quickly. Apologies for the trouble, and thanks for your patience and assistance.

[jdee]

@timursadykhov When you have a chance, can you please try the maintenance-1.1 branch again? I tested this on iOS and Android, with multiple subscriptions, unsubscribe/resubscribe. Everything worked well at app launch. Each subscribe callback was invoked once. It would be nice to know if this works with your app.

[timursadykhov]

@jdee thanks for the great explanation. I had to switch focus for a bit to deal with a bug so I haven't had a chance to try this out. Will follow up once I do.

[jdee]

@timursadykhov Have you had a chance to look at these recent changes? Since I couldn't reproduce the issue directly, I'm curious to know how/if it addresses your problem. I'll release this as 1.1.2 if you can confirm.

[timursadykhov]

@jdee So I finally had a chance to try out the new code and unfortunately it's not good news, I'm seeing the same issue. Do you need me to dig in and see what's happening?

[jdee]

@timursadykhov That's a shame. Can I just ask: Is it still happening 90% of the time? Is it sporadic?

There will always be a weakness because the call to retrieve the cache at initialization is asynchronous. If the API response from Branch happens to be returned at about the same time as the cache retrieval, there's a race between the promise and the live event. But that should not happen frequently or consistently.

I have some ideas to work around it, but since I can't consistently produce the problem that happens all the time for you, it's hard to know what will work.

Let me dig a little bit. I can simulate a large app loading by adding some setTimeouts here and there. If I can find a way produce your problem consistently, I should be able to fix it.

Sorry for the further delay. I'll have something else for you soon.

[timursadykhov]

@jdee in my testing today it's been happening 95% of the time. Once out of about 20 or 30 tries it did not get called twice.

[jdee]

@timursadykhov I'm still not able to reproduce this problem at all, but I think you're not the only one experiencing it. I have a few things to try, but I don't want to keep wasting your time. It would be good to have a better idea of what's going on. Would you be interested in doing a screen-sharing session? Maybe I can see things more clearly if I can actually see what's happening in your app. Please contact me at [email protected] to set this up if you'd like to do this.

[amsdamsgram]

Hey @jdee,

I'm also facing this issue on Android. Callback of subscribe is called twice every time I open the app for the first time. Also tried maintenance-1.1 branch but it's still happening :/

Using RN 0.43.2 and branch.io 2.0 on Android 7.0

[jdee]

Hi @iDams. I'm sorry this is affecting you too. I spent some time with @timursadykhov the other day and found a solution that eliminated the problem. He wasn't able to produce it in at least ten tries. I want to review that solution thoroughly, which is why it hasn't been released yet. But since this is clearly not an isolated case, I'll get something into the master branch by the end of the day and release 2.0.0-beta.2 with the fix by the end of the week. If the fix proves to be solid, I can also release a patch to 1.1 and maybe 0.9. I appreciate your patience.

[amsdamsgram]

@jdee No worries ;)
Thanks for your work on it.

[jdee]

@iDams @timursadykhov 2.0.0-beta.2 is out with these changes. I'm skeptical because it's been so hard for me to produce and so persistent for you. I'm eager to know how this works for you.

[amsdamsgram]

@jdee It fixed it for me. Nice work! Thanks

EDIT: it broke testing with jest though

Invariant Violation: Native module cannot be null.

      at invariant (node_modules/fbjs/lib/invariant.js:44:15)
      at new NativeEventEmitter (node_modules/react-native/Libraries/EventEmitter/NativeEventEmitter.js:32:1)
      at new Branch (node_modules/react-native-branch/src/index.js:27:219)
      at Object.<anonymous> (node_modules/react-native-branch/src/index.js:106:1)
      at Object.<anonymous> (src/containers/HomePage/index.js:4:24)

I used to do this to fix the issue but it doesn't seem to work anymore:

// .jestsetup.js
React.NativeModules.RNBranch = {};

[jdee]

Hi @iDams. It's good news that this fixed the problem for you. I want to review this fix a little more and then I'll probably release it as 1.1.2.

You can easily fix your mock issue, at least temporarily, by adding:

React.NativeModules.RNBranchEventEmitter = {};

But I think the right idea would be instead to mock the call to branch.subscribe instead of mocking the NativeModules. I really need to find a good mock framework for JS. It looks like jack had the right idea, but it hasn't been updated in five years and requires node < 0.7.

In order to simulate certain things in unit tests for these recent additions, I did some ad hoc mocking. See things like https://github.com/BranchMetrics/react-native-branch-deep-linking/blob/master/test/index.js#L45 and https://github.com/BranchMetrics/react-native-branch-deep-linking/blob/master/test/index.js#L81. The tests temporarily modify the NativeModules with mock methods and then restore the original methods and the end of the test. There are still some problems with those tests as they stand, but this basically works.

You could take a similar approach with branch, but you're not the only one who needs this. Maybe I can at least provide a branch mock you can plug into your unit tests so that it doesn't try to load NativeModules and just lets you call subscribe and other methods without actually doing anything.

[amsdamsgram]

Thanks @jdee

I did this in the end:

React.NativeModules.RNBranch = {
  redeemInitSessionResult: jest.fn().mockImplementation(() => Promise.resolve())
};

React.NativeModules.RNBranchEventEmitter = {};

But yeah you're right, the proper way should be to mock to subscribe.
Thanks for your exemple, it helped a lot.

[timursadykhov]

@jdee sorry to be so slow in getting back to you. I finally did some testing with 2.0.0-beta.3 and I didn't see the issue in my android build a single time. It looks good to me!

[jdee]

Thanks @timursadykhov, and apparently I'm also slow.

I think backporting this fix to 1.1 or 0.9 will be tricky, and 2.0 is nearing a production release. If anyone desperately needs it, please comment/reopen or open a new issue. For now, I'm considering this issue closed.

[knitish90]

I am getting the same issue, may I get the correct solution?