RCTModuleData deadlock when running with debugger

[bruzenak]

Description

When running with the debugger in 0.38, RCTModuleData deadlocks on line 88 of RCTModuleData.mm (std::unique_lockstd::mutex lock(_instanceLock);). This reproduces reliably with my setup.

From a layman's read, it appears that a module I'm using (react-native-search-bar) is set up to load off the main thread. While it's doing this and grabbing RCTAccessibilityManager, the main thread reloads and tries to grab the lock. Meanwhile RCTAccessibilityManager wants the main thread and already HAS the lock. So it's waiting on dispatch to the main thread, and the main thread is waiting on the accessibility manager to load.

Without the debugger, this doesn't lock. It appears the debugger attaches and triggers a reload in [RCTDevMenu setExecutorClass:], and that reload is what's causing this lock.

Reproduction

I attached a package.json file that reproduces this issue.

1. Run react-native init BugSample
   1a. Drop in package.json
   1b. Run npm i
2. Open iOS project in XCode
3. Hit run
4. Attach debugger (Cmd+D)
5. Quit
6. Restart

This project reliably deadlocks for me.

package.json.zip

Solution

Don't grab the locks; wait on the full load process to complete before attaching the debugger, etc. Something this close to core should probably be fixed by an engineer that fully understands the module loading stack.

Additional Information

• React Native version: 0.38.0
• Platform: iOS (only tested on iOS)
• Operating System: MacOS

[lacker]

That sounds like it would be a fairly disruptive change, to not grab the locks here. Is there any simpler solution? I am afraid that the cost of fixing this would be above the cost of leaving it as is, unless a bunch more people show up with this issue.

[bruzenak]

Well, it's definitely a deadlock. I'm sure there's a better fix than not grabbing the locks there, but it requires a deeper knowledge of how the library loading works than I have at the moment. For example: requiring that the lock gets grabbed before a reload can be triggered might fix it.

Certainly the current behavior is bad in that it does nothing to prevent deadlocking between two threads. The cost of not fixing it would be that this deadlocks in other situations that don't require the debugger, which I'm betting are possible. I'm gonna guess that whoever put the logger a few lines up warning about the deadlock was looking for an easy way to reproduce.

[Luzgan]

Well "unless a bunch more people show up with this issue" - I cannot ignore thing like that...

2017-01-31 12:55:40.643 [warn][tid:com.facebook.react.RCTBridgeQueue][RCTModuleData.mm:220] RCTBridge required dispatch_sync to load RCTAccessibilityManager. This may lead to deadlocks
2017-01-31 12:55:44.705 [fatal][tid:com.facebook.react.RCTBridgeQueue] Runtime is not ready for debugging.

Basically it is disallowing me to work with debugger at all, because for me it is happening when I click Debug JS and application tries to reload.

Sadly because of time I won't be able to give much more information in nearest future, just wanted to say something to not make this forgotten. I am using RN0.40.

[hongkim91]

I have the same deadlock issue when trying to use react-native-youtube . I'm using RN 0.41.2

[lacker]

Since all the bug repros involve specific libraries, maybe it would be more effective to report this issue to the makers of react-native-search-bar and react-native-youtube?

[bruzenak]

When I dug into this, it was not a client library bug, it was a React Native core bug. I imagine you'll see a variety of libraries trigger it based on hitting the race condition in the code. I don't think there's anything those libraries could do to fix it except putting in random arbitrary waits.

There's always the possibility that this is client related, but nothing I've seen in the client code is doing anything that I'd consider out of the ordinary for module loading.

[lacker]

Hmm. I believe you, I am just wondering how to make it simpler for people to be able to debug this. Is there some way to reproduce this without any third-party modules?

[bruzenak]

You could probably build a module that exhibited the behavior as part of a project. I'd dig in further but am unfortunately strapped for time. Race conditions suck. Maybe if you built some code to purge the module cache and reloaded a few times?

[ApolloMuses]

Update: I noticed the deadlock only occurs for me when I am working on a branch. Each time I submit a pull request and merge into the master, the deadlock is resolved. Maybe this will help some of you.

[kevando]

No idea what I'm doing, but I get this same error when I enable the chrome debugger and use this lib

https://github.com/gre/react-native-view-shot

:/

[ds300]

I'm also getting this with react-native-search-bar. Anybody found a workaround?

[rkretzschmar]

Hi all,
for me - the following solved the problem (deadlock):
I patched RCTModuleData.mm by replacing the mutex in setUpInstanceAndBridge in a way that it only protects the write access (the new operation in this case), rather than all the block - so the method looks like that now:

- (void)setUpInstanceAndBridge
{
  RCT_PROFILE_BEGIN_EVENT(RCTProfileTagAlways, @"[RCTModuleData setUpInstanceAndBridge] [_instanceLock lock]", @{ @"moduleClass": NSStringFromClass(_moduleClass) });
  
  if (!_setupComplete && _bridge.valid) {
    if (!_instance) {
      if (RCT_DEBUG && _requiresMainQueueSetup) {
        RCTAssertMainQueue();
      }
      
      // Only the write access (the new operation in this case) has to be protected really.
      {
        std::unique_lock<std::mutex> lock(_instanceLock);
        
        // This IF-statement is still necessary, because the instance could have already been instantiated while this thread was waiting
        if (!_instance) {
          RCT_PROFILE_BEGIN_EVENT(RCTProfileTagAlways, @"[RCTModuleData setUpInstanceAndBridge] [_moduleClass new]",  @{ @"moduleClass": NSStringFromClass(_moduleClass) });
          _instance = [_moduleClass new];
          RCT_PROFILE_END_EVENT(RCTProfileTagAlways, @"");
        }
      }
      
      if (!_instance) {
        // Module init returned nil, probably because automatic instantatiation
        // of the module is not supported, and it is supposed to be passed in to
        // the bridge constructor. Mark setup complete to avoid doing more work.
        _setupComplete = YES;
        RCTLogWarn(@"The module %@ is returning nil from its constructor. You "
                   "may need to instantiate it yourself and pass it into the "
                   "bridge.", _moduleClass);
      }
    }
    
    if (_instance && RCTProfileIsProfiling()) {
      RCTProfileHookInstance(_instance);
    }
    
    // Bridge must be set before methodQueue is set up, as methodQueue
    // initialization requires it (View Managers get their queue by calling
    // self.bridge.uiManager.methodQueue)
    [self setBridgeForInstance];
  }
  
  [self setUpMethodQueue];
  
  RCT_PROFILE_END_EVENT(RCTProfileTagAlways, @"");
  
  // This is called outside of the lock in order to prevent deadlock issues
  // because the logic in `finishSetupForInstance` can cause
  // `moduleData.instance` to be accessed re-entrantly.
  if (_bridge.moduleSetupComplete) {
    [self finishSetupForInstance];
  } else {
    // If we're here, then the module is completely initialized,
    // except for what finishSetupForInstance does.  When the instance
    // method is called after moduleSetupComplete,
    // finishSetupForInstance will run.  If _requiresMainQueueSetup
    // is true, getting the instance will block waiting for the main
    // thread, which could take a while if the main thread is busy
    // (I've seen 50ms in testing).  So we clear that flag, since
    // nothing in finishSetupForInstance needs to be run on the main
    // thread.
    _requiresMainQueueSetup = NO;
  }
}

I know that mutex is not the root cause and a deadlock can still be happen, because of all those dispatch_sync (RCTUnsafeExecuteOnMainQueueSync) calls. But it may at least decreases the chance of a deadlock.

Please - can somebody verify that?

Thanks in advance.

[rkretzschmar]

I also protected setBridgeForInstance and setUpMethodQueue with their own mutex:

Having:

  std::mutex _instanceLock;
  std::mutex _bridgeLock;
  std::mutex _methodQueueLock;

setBridgeForInstance looks like this now:

- (void)setBridgeForInstance
{
  if ([_instance respondsToSelector:@selector(bridge)] && _instance.bridge != _bridge) {
    RCT_PROFILE_BEGIN_EVENT(RCTProfileTagAlways, @"[RCTModuleData setBridgeForInstance]", nil);
    {
      std::unique_lock<std::mutex> lock(_bridgeLock);
      
      if ([_instance respondsToSelector:@selector(bridge)] && _instance.bridge != _bridge) {
        @try {
          [(id)_instance setValue:_bridge forKey:@"bridge"];
        }
        @catch (NSException *exception) {
          RCTLogError(@"%@ has no setter or ivar for its bridge, which is not "
                      "permitted. You must either @synthesize the bridge property, "
                      "or provide your own setter method.", self.name);
        }
      }
    }
    RCT_PROFILE_END_EVENT(RCTProfileTagAlways, @"");
  }
}

and setUpMethodQueue like this:

- (void)setUpMethodQueue
{
  if (_instance && !_methodQueue && _bridge.valid) {
    
    RCT_PROFILE_BEGIN_EVENT(RCTProfileTagAlways, @"[RCTModuleData setUpMethodQueue]", nil);
    BOOL implementsMethodQueue = [_instance respondsToSelector:@selector(methodQueue)];
    if (implementsMethodQueue && _bridge.valid) {
      _methodQueue = _instance.methodQueue;
    }
    if (!_methodQueue && _bridge.valid) {
      
      // Create new queue (store queueName, as it isn't retained by dispatch_queue)
      _queueName = [NSString stringWithFormat:@"com.facebook.react.%@Queue", self.name];
      _methodQueue = dispatch_queue_create(_queueName.UTF8String, DISPATCH_QUEUE_SERIAL);

      // assign it to the module
      if (implementsMethodQueue) {
        
        {
          std::unique_lock<std::mutex> lock(_methodQueueLock);
          
          if ([_instance respondsToSelector:@selector(methodQueue)] && !_instance.methodQueue) {
            @try {
              [(id)_instance setValue:_methodQueue forKey:@"methodQueue"];
            }
            @catch (NSException *exception) {
              RCTLogError(@"%@ is returning nil for its methodQueue, which is not "
                          "permitted. You must either return a pre-initialized "
                          "queue, or @synthesize the methodQueue to let the bridge "
                          "create a queue for you.", self.name);
            }
          }
        }
      }
    }
    RCT_PROFILE_END_EVENT(RCTProfileTagAlways, @"");
  }
}

Still working without deadlock.

[vikeri]

@rkretzschmar If you can generate a patch file then I'll happily try it :)

[rkretzschmar]

Hi @vikeri - thanks!
Here is the commit rkretzschmar@d0a9c7d based on the latest react-native master. Maybe you can just cherry-pick.

[vikeri]

@rkretzschmar Awesome! Thanks, I'll try it asap

[joonhocho]

I am having the same issues. React native v0.42.3

2017-03-31 04:38:31.533 [warn][tid:com.facebook.react.RCTBridgeQueue][RCTModuleData.mm:285] Required dispatch_sync to load constants for RCTSourceCode. This may lead to deadlocks

[rkretzschmar]

Hi @joonhocho - sorry to hear that. Do you think you have time to try out my patch rkretzschmar@d0a9c7d ?

[mikesurowiec]

This problem had been plaguing me for awhile, I was finally able to fix it by re-ordering the Build Phases section. I moved libRNSearchBar.a to the bottom of my build phases and now I can reload JS with the debugger running. Hope this helps someone!

[xuetongiqn]

@mikesurowiec it works for me, Thank you!

[grabbou]

Still happens, had to remove that module.

[hramos]

Hi there! This issue is being closed because it has been inactive for a while. Maybe the issue has been fixed in a recent release, or perhaps it is not affecting a lot of people. Either way, we're automatically closing issues after a period of inactivity. Please do not take it personally!

If you think this issue should definitely remain open, please let us know. The following information is helpful when it comes to determining if the issue should be re-opened:

• Does the issue still reproduce on the latest release candidate? Post a comment with the version you tested.
• If so, is there any information missing from the bug report? Post a comment with all the information required by the issue template.
• Is there a pull request that addresses this issue? Post a comment with the PR number so we can follow up.

If you would like to work on a patch to fix the issue, contributions are very welcome! Read through the contribution guide, and feel free to hop into #react-native if you need help planning your contribution.

[waifo]

when i am running a new project the simulator works fine, but after some time on reloading the simulator screen turns black and the installed app closes.
I am not able to figure out the real issue.

[simlmx]

Unlike @mikesurowiec I didn't have any libRNSearchBar.a library but moving some other third-party libraries to the end fixed it for me.