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

fix: Safely and lazily initialize native RNSkia JSI Bindings #151

Merged
merged 10 commits into from
Feb 1, 2022

Conversation

mrousavy
Copy link
Contributor

@mrousavy mrousavy commented Jan 28, 2022

The current way RNSkia JSI Bindings are installed is unsafe and actually crashes in Release builds on Android, since it is not following JSI Runtimes Threading Guidelines.

A JSI Runtime has no Thread Safety, and RNSkia does not enforce initial JSI Runtime operations (install the SkiaApi global) to run on the JS Thread, and since they are called when the RN Skia Module initializes, they are actually ran on the Native Module (mqt_native/mqt_nm) Thread, which leads to crashes since the Runtime might not be available yet.

This PR changes this behavior to expose a install(...) function that uses React Native's "blocking synchronous" native module function API, which as I initially expected, executes on the JS Thread (it's blocking.). This function is then called in the top-level index.ts file.

Benefits

  • Doesn't randomly crash on iOS release builds anymore
  • Doesn't randomly crash on Android release builds anymore
  • Seems to be safer and more predictable because we install the API just before we use it
  • A bit more lazy since the JSI Bindings only get installed once the react-native-skia package is imported, as opposed to eagerly install on app launch

@mrousavy mrousavy changed the title fix: Safely and lazily initialize native RNSkia Bindings fix: Safely and lazily initialize native RNSkia JSI Bindings Jan 28, 2022
Copy link
Contributor

@chrfalch chrfalch left a comment

Choose a reason for hiding this comment

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

This looks awesome! Will test and verify very soon!

@mrousavy
Copy link
Contributor Author

Cool - as discussed on Slack, it works for me on iOS. I unfortunately couldn't build Android, but I think the code is right. Feel free to DM me to troubleshoot if something isn't working there.

@chrfalch
Copy link
Contributor

Cool - as discussed on Slack, it works for me on iOS. I unfortunately couldn't build Android, but I think the code is right. Feel free to DM me to troubleshoot if something isn't working there.

I'll test on both platforms to make sure it is working. Thanks so much!! <3

@chrfalch
Copy link
Contributor

Fixed a missing import on Android (ReactMethod), and merged with main.

Copy link
Contributor

@chrfalch chrfalch left a comment

Choose a reason for hiding this comment

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

Upgraded with latest from main, fixed missing import on Android.

Tested on both Android/iOS and it works fine.

@axeldelafosse
Copy link

Woohoo! Thanks Marc!

@mrousavy
Copy link
Contributor Author

glad it works out so well, can't believe I didn't discover this sooner for MMKV 🙈

throw new Error(
`Native Skia Module failed to correctly install JSI Bindings! Result: ${result}`
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You can replace it with :

if (!global.SkiaApi) {
  // Initialize RN Skia
  const SkiaModule = NativeModules.RNSkia;
  if (!SkiaModule || typeof SkiaModule.install !== "function") {
    throw new Error(
      "Native RNSkia Module cannot be found! Make sure you correctly " +
        "installed native dependencies and rebuilt your app."
    );
  }
  const result = SkiaModule.install();
  if (!result) {
    throw new Error(
      `Native Skia Module failed to correctly install JSI Bindings! Result: ${result}`
    );
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol I know that I can, but I won't because those implicit boolean checks are hard to read.

@chrfalch
Copy link
Contributor

glad it works out so well, can't believe I didn't discover this sooner for MMKV 🙈

This will become the new pattern for safely initialising JSI Modules!!!

@wcandillon wcandillon merged commit 770138d into Shopify:main Feb 1, 2022
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.

6 participants