realm / realm-js

Realm is a mobile database: an alternative to SQLite & key-value stores
https://realm.io
Apache License 2.0
5.72k stars 564 forks source link

Throwing in a Results listener callback crashes the app #2654

Open kraenhansen opened 4 years ago

kraenhansen commented 4 years ago

Goals

Realm should never crash a React Native app.

I suspect that we need to tell the JSCore engine that an unhandled exception happened during the execution of the callback.

I suspect this also happens if the error is thrown from other callbacks that we call from C++, therefore the following issues probably share the same root cause: https://github.com/realm/realm-js/issues/2029, https://github.com/realm/realm-js/issues/1439, https://github.com/realm/realm-js/issues/2233.

Expected Results

When an error is thrown from a callback added as a listener on a Results object, I expect an uncaught exception to be handled (resulting in a black screen with a red top-bar while in development mode).

Actual Results

When an error is thrown from a callback added as a listener on a Results object, the app crashes!

Steps to Reproduce & Code Sample

// App.js

import React, { Component } from 'react';
import { View, Text } from 'react-native';
import Realm from 'realm'

export default class App extends Component {
  componentDidMount() {
    const realm = new Realm({
        schema: [{
            name: "Singleton",
            properties: { value: "string" },
        }],
    });

    realm.objects("Singleton").addListener(() => {
        throw new Error("Change happened!");
    });
  }

  render() {
    return <View><Text>App loaded ...</Text></View>;
  }
}

Version of Realm and Tooling

kneth commented 4 years ago

It might be out of the scope of Realm as a library to make the decision to swallow exceptions. Other options exist within the React Native framework:

I don't consider the current behaviour as a bug.

kraenhansen commented 4 years ago

Realm should never crash a React Native app.

I don't consider the current behaviour as a bug.

It might not be straight forward to understand or fix, but let's not kid ourselves: Of cause it's a bug when a library breaks invariants of the environment, namely "JS code might throw unhandled exceptions, but it will never crash the process running the JS code". It is super unexpected behavior on our part.

kneth commented 4 years ago

I agree that the correct behaviour is that a JS exception is thrown: a raw C++ exception is definitely not adding any value for the app developer.

kneth commented 4 years ago

After 6 months I still don't consider it a bug. Throwing an exception inside a callback is not best practise. Moreover, callbacks should have a try/catch and handle errors gracefully. We should document it clearly in our API documentation that we except users to handle errors inside the listener.

steffenagger commented 4 years ago

@kneth is the error actually thrown in JS-context? If the error doesn't currently show up as a "red error screen" in React Native development mode (that's how I read the initial issue), it sounds like the app crashed at a level where developers can't catch the error.

kneth commented 2 years ago

We will document the behaviour until we have a clear idea of how to solve it.