search

facebook / react-native

A framework for building native applications using React
https://reactnative.dev
MIT License
119.02k stars 24.31k forks source link

[iOS] Subscriptions to RCTEventEmitter not cleaned up during reload #41188

Closed gregbrinker closed 1 year ago

gregbrinker commented 1 year ago

The Issue

Hello! The last few months, my team has noticed that sometimes when the app is reloaded, it crashes. It didn't affect anything in production, so we just let it be. However we finally got annoyed enough with it that we decided to start looking into it. It turns out that in our case, it's due to the RNSensorsGravity module from react-native-sensors

Here's the error that's logged in Xcode:

*** Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'Error when sending event: RNSensorsGravity with body: {
    timestamp = 1698244972276;
    x = "-0.02071463316679001";
    y = "-0.0002588966453913599";
    z = "-0.9997854232788086";
}. RCTCallableJSModules is not set. This is probably because you've explicitly synthesized the RCTCallableJSModules in RNSensorsGravity, even though it's inherited from RCTEventEmitter.'
*** First throw call stack:
(0x19a1c5e88 0x1934f38d8 0x194ae1c88 0x1015bc570 0x101550158 0x1946432f0 0x1946174a0 0x194617430 0x1945d87d8 0x1945d850c 0x1945ddc20 0x108da31d0 0x108d9205c 0x108da2810 0x108da2354 0x19a2566f8 0x19a238058 0x19a23ced4 0x1d353a368 0x19c71b3d0 0x19c71b034 0x100a755c8 0x1b88a4960)
libc++abi: terminating with uncaught exception of type NSException

This error is from the RCTAssert in RCTEventEmitter:

RCTAssert(
      _callableJSModules != nil,
      @"Error when sending event: %@ with body: %@. "
       "RCTCallableJSModules is not set. This is probably because you've "
       "explicitly synthesized the RCTCallableJSModules in %@, even though it's inherited "
       "from RCTEventEmitter.",
      eventName,
      body,
      [self class]);

Investigation

I searched around and found many issues related to this, but none were helpful:

So I started digging more into the native code. Immediately before the code that sends the event over the bridge, I added log:

NSLog(@"[TESTING] SENDING EVENT %@", self.callableJSModules != nil ? @"NOT NIL" : @"NIL");
[self sendEventWithName:@"RNSensorsGravity" body:@{
                @"x" : [NSNumber numberWithDouble:x],
                @"y" : [NSNumber numberWithDouble:y],
                @"z" : [NSNumber numberWithDouble:z],
                @"timestamp" : [NSNumber numberWithDouble:timestamp]
            }];

Here's what it logged out. The last log before the crash shows that self.callableJSModules = nil, which triggers the RCTAssert to crash:

[TESTING] SENDING EVENT NOT NIL
[TESTING] SENDING EVENT NOT NIL
[TESTING] SENDING EVENT NOT NIL
[TESTING] SENDING EVENT NIL

I was curious, so I wrapped the [self sendEventWithName ...] in a check to ensure that self._callableJSModules isn't nil:

NSLog(@"[TESTING] SENDING EVENT %@", self.callableJSModules != nil ? @"NOT NIL" : @"NIL");
if (self.callableJSModules != nil) {
    [self sendEventWithName:@"RNSensorsGravity" body:@{
        @"x" : [NSNumber numberWithDouble:x],
        @"y" : [NSNumber numberWithDouble:y],
        @"z" : [NSNumber numberWithDouble:z],
        @"timestamp" : [NSNumber numberWithDouble:timestamp]
    }];
}

Something interesting happens here. After I reload the app, here's what is logged:

SENDING EVENT NOT NIL
SENDING EVENT NOT NIL
SENDING EVENT NOT NIL // <-- last log before reloading app
SENDING EVENT NIL
SENDING EVENT NOT NIL
SENDING EVENT NIL

And then if I reload a 3rd time...

SENDING EVENT NIL
SENDING EVENT NIL
SENDING EVENT NOT NIL
SENDING EVENT NIL
SENDING EVENT NIL
SENDING EVENT NOT NIL
SENDING EVENT NIL
SENDING EVENT NIL
SENDING EVENT NOT NIL

It appears that the RCTEventEmitter for RNSGravity are being created, but never cleaned up. In our code, we have a useEffect that subscribes to the event, and on unmount removes the subscription:

import { gravity } from "react-native-sensors"

...

useEffect(() => {
  // Subscribe to gravity
  const subscription = gravity.subscribe(
    result => {
      console.log("Gravity: ", result)
    },
    err => {
      console.warn("gravity sensor error", err)
    },
  )

  // Unsubscribe on unmount
  return () => {
    subscription.unsubscribe()
  }
}, [])

The unsubscribe() never gets called, but the Native Module continues attempting to send events over the bridge. Every time the app is reloaded, a fresh RCTEventEmitter is created, which works fine - but the old ones are stale and would trigger crashes had I not wrapped the sendEventWithName code in the check for self._callableJSModules.

Solution?

I'm not quite sure how to accomplish it, but I believe I need to find a way to "destroy" RCTEventEmitters when the bridge is reloaded. I attempted to add listeners to RCTEventEmitter to call invalidate whenever the bridge is reloaded, but that didn't work:

- (id) init
{
    self = [super init];
    RCTBridge *bridge = [RCTEventEmitter getRCTBridge];
    [[NSNotificationCenter defaultCenter] addObserver:self
                                               selector:@selector(invalidate)
                                                   name:RCTBridgeWillReloadNotification
                                                 object:bridge];
    return self;
}

I mentioned earlier that this is only a problem in DEV, because the prod app rarely restarts. The one time that it does that I can think of is during a CodePush, where [super.bridge reload] is called.

I'm hoping that someone knows of a way to accomplish this. I imagine that this is an issue for any library that relies on subscriptions.

Thanks!

React Native Version

0.70.8

Output of npx react-native info

System: OS: macOS 13.0 CPU: (8) arm64 Apple M1 Pro Memory: 72.22 MB / 16.00 GB Shell: 5.8.1 - /bin/zsh Binaries: Node: 18.12.0 - ~/.volta/tools/image/node/18.12.0/bin/node Yarn: Not Found npm: 8.19.2 - ~/.volta/tools/image/node/18.12.0/bin/npm Watchman: 2023.02.20.00 - /opt/homebrew/bin/watchman Managers: CocoaPods: 1.12.0 - /usr/local/bin/pod SDKs: iOS SDK: Platforms: DriverKit 22.4, iOS 16.4, macOS 13.3, tvOS 16.4, watchOS 9.4 Android SDK: API Levels: 29, 30, 31, 32, 33 Build Tools: 30.0.2, 30.0.3, 31.0.0, 32.0.0, 33.0.2 System Images: android-29 | Intel x86 Atom_64, android-29 | Google APIs ARM 64 v8a, android-32 | Google APIs ARM 64 v8a, android-33 | Google APIs ARM 64 v8a Android NDK: Not Found IDEs: Android Studio: 2022.1 AI-221.6008.13.2211.9619390 Xcode: 14.3/14E222b - /usr/bin/xcodebuild Languages: Java: 11.0.13 - /usr/bin/javac npmPackages: @react-native-community/cli: Not Found react: 18.1.0 => 18.1.0 react-native: 0.70.8 => 0.70.8 react-native-macos: Not Found npmGlobalPackages: react-native: Not Found

Steps to reproduce

  1. Start subscribing to any NativeModule that subclasses RCTEventEmitter (In my case, react-native-sensors - but any subscription would do)
import { gravity } from "react-native-sensors"
...
useEffect(() => {
  // Subscribe to gravity
  const subscription = gravity.subscribe(
    result => {
      console.log("Gravity: ", result)
    },
    err => {
      console.warn("gravity sensor error", err)
    },
  )

  // Unsubscribe on unmount
  return () => {
    subscription.unsubscribe()
  }
}, [])
  1. Trigger reload of app by hitting R in terminal, or "Reload" in the Dev Menu
  2. Observe error

Snack, screenshot, or link to a repository

It technically doesn't run online because it isn't a real device, but here's what the code would look like

https://snack.expo.dev/I6s2wglbh

github-actions[bot] commented 1 year ago
:warning: Newer Version of React Native is Available!
:information_source: You are on a supported minor version, but it looks like there's a newer patch available - 0.70.14. Please upgrade to the highest patch for your minor or latest and verify if the issue persists (alternatively, create a new project and repro the issue in it). If it does not repro, please let us know so we can close out this issue. This helps us ensure we are looking at issues that still exist in the most recent releases.
zhongwuzw commented 1 year ago

@gregbrinker I think it's because sensors lib has retain cycle, can you try this PR? https://github.com/react-native-sensors/react-native-sensors/pull/458

gregbrinker commented 1 year ago

@zhongwuzw giving that a try!

gregbrinker commented 1 year ago

@zhongwuzw that worked! Amazing - thanks so much. Looks like this isn't a React Native issue, but rather an issue with that library in particular