Disable flaky RNTester test

Summary:
`[RCTBridge setUp]` and `[RCTBridge invalidate]` execute asynchronously and concurrently. Therefore, it's not safe to call one method after the other, as we do in `[RCTBridge reload]`.

In this test, we create a bridge, and immediately reload it. Initializing the bridge causes the JS bundle to execute. Invalidating the bridge causes the jsThread to be terminated. If circumstances are correct, we could end up trying to executing the JS bundle after the jsThread has been terminated, which can lead to these assertions being triggered:
1. `RCTAssert(_jsThread, @"This method must not be called before the JS thread is created");` in `ensureOnJavaScriptThread:`.
2. `RCTAssert(_jsMessageThread != nullptr, @"Cannot invoke completion without jsMessageThread");` in `enqueueApplicationScript:url:onComplete:`.

```
- (void)testUnderlyingBridgeIsDeallocated
{
  RCTBridge *bridge;
  __weak id batchedBridge;
  autoreleasepool {
    bridge = [[RCTBridge alloc] initWithBundleURL:_bundleURL moduleProvider:nil launchOptions:nil];
    batchedBridge = bridge.batchedBridge;
    XCTAssertTrue([batchedBridge isValid], @"RCTBridge impl should be valid");
    [bridge reload];
  }

  RCT_RUN_RUNLOOP_WHILE(batchedBridge != nil)

  XCTAssertNotNil(bridge, @"RCTBridge should not have been deallocated");
  XCTAssertNil(batchedBridge, @"RCTBridge impl should have been deallocated");

  // Wait to complete the test until the new bridge impl is also deallocated
  autoreleasepool {
    batchedBridge = bridge.batchedBridge;
    [bridge invalidate];
    bridge = nil;
  }

  RCT_RUN_RUNLOOP_WHILE(batchedBridge != nil);
  XCTAssertNil(batchedBridge);
}
```

To verify that this race is real, patch: P62410422. This adds an artificial delay in the `[RCTCxxBridge start]` method, which makes it so that the bridge is invalidated and the js thread is destroyed before we start executing the jsBundle.

I think a proper solution to this problem would require some bit of restructuring of `[RCTCxxBridge invalidate]` and `[RCTCxxBridge start]` to either:
1. Force `[RCTCxxBridge invalidate]` to wait for `[RCTCxxBridge start]` to complete and vice versa.
2. Make it safe to interleave execution of `[RCTCxxBridge start]` and `[RCTCxxBridge invalidate]`.

I tried the first approach using two semaphores: `_startSem(1)` and `_invalidateSem(0)`. When you start executing the code inside `[RCTCxxBridge start]`, you `semWait(_startSem)`. When you stop executing the code inside `[RCTCxxBridge start]` (which could happen in another thread at some later point in time), you `semSignal(_invalidateSem)`. Likewise, when you start executing `[RCTCxxBridge invalidate]`, you `semWait(_invalidateSem)` and when you stop executing the code inside `[RCTCxxBridge invalidate]` you `semSignal(_startSem)`. This way, invalidates always wait for starts to finish, and starts always wait for invalidates to finish. But considering all the concurrency involved in these methods, this is hard to get right.

The second approach seems possible. You could keep locks for the shared data, and create critical sections whever you want to access that data. I didn't actually try to implement this approach though.

Given that we're going to elminate the Bridge anyway, and that this race condition practically only occurs when you reload imediately after initializing the bridge (which can only really be done programmatically), I think it's fine to just disable the test for now. One other thing I considered was making the current thread sleep for some time after we created the bridge in the test. The reason why I'm hesitant to implement this approach is that it would slow down the execution of the test suite and still wouldn't guarantee that we don't hit this race condition. Ultimately, our infra might end up disabling these tests again.

Reviewed By: shergin

Differential Revision: D14909121

fbshipit-source-id: d7d441c3e2f0ad59182c8c7e23740be4ac4cf83c
This commit is contained in:
Ramanpreet Nara
2019-04-12 14:38:16 -07:00
committed by Facebook Github Bot
parent a789ec6193
commit 108c4190ed

View File

@@ -169,7 +169,17 @@ RCT_EXPORT_METHOD(test:(__unused NSString *)a
#endif
}
- (void)testUnderlyingBridgeIsDeallocated
/**
* T42930872:
*
* Both bridge invalidation and bridge setUp occurr execute concurrently.
* Therefore, it's not safe for us to create a bridge, and immediately reload on
* it. It's also unsafe to just reload the bridge, because that calls invalidate
* and then setUp. Because of these race conditions, this test may randomly
* crash. Hence, we should disable this test until we either fix the bridge
* or delete it.
*/
- (void)disabled_testUnderlyingBridgeIsDeallocated
{
RCTBridge *bridge;
__weak id batchedBridge;