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

Use RCTBridgeModule's bridge property instead of [RCTBridge currentBridge] #249

Merged

Conversation

cltnschlosser
Copy link
Contributor

From the docs of RCTBridge:

/**
 * The last current active bridge instance. This is set automatically whenever
 * the bridge is accessed. It can be useful for static functions or singletons
 * that need to access the bridge for purposes such as logging, but should not
 * be relied upon to return any particular instance, due to race conditions.
 */
+ (instancetype)currentBridge
{
  return RCTCurrentBridgeInstance;
}

What I've found is there exists a race condition where a JS reload triggered very early could potentially result in MMKV install failing in the initial JS context. Much of this is due to the bridge invalidation being asynchronous. It looks something like this:

  • App launch
  • Bridge 1 is created
  • currentBridge set to bridge 1
  • Bridge 1 runtime is initialized (start JS)
  • *Reload
  • Bridge 1 invalidation start
  • currentBridge set to nil
  • Bridge 2 is created
  • currentBridge is set to bridge 2
  • *MMKV from bridge 1 calls install (get’s currentBridge from bridge 2)
    Arbitrary order:
  • MMKV install error thrown from bridge 1
  • Bridge 1 invalidation finishes (JS stops running)
  • Bridge 2 runtime is initialized (start JS)

*Very precisely placed steps to trigger this race condition

Realistically we don't care that the MMKV install in bridge 1 failed, except for the fact that it will throw an error (which can crash the app). Using _bridge we have access the correct bridge, even if it's invalidation has started. The install work in the bridge 1 context may be unnecessary and will be later thrown away when the invalidation is finished, but at least it doesn't error. Later MMKV install is called in bridge 2's context and this is what will be used going forward.

@vercel
Copy link

vercel bot commented May 9, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
rnmmkv ✅ Ready (Inspect) Visit Preview May 9, 2022 at 7:29PM (UTC)


RCTBridge* bridge = [RCTBridge currentBridge];
RCTCxxBridge* cxxBridge = (RCTCxxBridge*)bridge;
RCTCxxBridge* cxxBridge = (RCTCxxBridge*)_bridge;
Copy link
Contributor Author

@cltnschlosser cltnschlosser May 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_bridge is defined on line 15 @synthesize bridge = _bridge; this follows the documentation here https://github.com/facebook/react-native/blob/7c581f3d3007954413d68daf2e868ce93e120615/React/Base/RCTBridgeModule.h#L159

/**
 * A reference to the RCTBridge. Useful for modules that require access
 * to bridge features, such as sending events or making JS calls. This
 * will be set automatically by the bridge when it initializes the module.
 * To implement this in your module, just add `@synthesize bridge = _bridge;`
 * If using Swift, add `@objc var bridge: RCTBridge!` to your module.
 */
@property (nonatomic, weak, readonly) RCTBridge *bridge;

@ammarahm-ed
Copy link
Owner

Thanks for debugging this and sending a fix! Looks good.

@ammarahm-ed ammarahm-ed merged commit 7adcde4 into ammarahm-ed:master May 10, 2022
@cltnschlosser cltnschlosser deleted the cs_install-bridge-safety branch May 10, 2022 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants