Don't call out to the default handler in production mode

[nevir]

A continuation of the discussion in #515 and one way to fix #468 - however, this is a controversial change!

---

There are a few options as I see it (when in react native's production mode):

1. Don't call the native handler, ever (this PR).
   • Upside is that Raven can always send exceptions (assuming connectivity; also see React Native plugin ready for production #489 (comment))
   • Downside is that you lose the benefit of aggressively crashing: The app could now be in an inconsistent state
2. Call the native handler after sending the exception, or timing out
   • Upside is that this behaves more closely to how RN behaves out of the box
   • Downside is that the app could be in an inconsistent state (a few seconds is a long time!)
3. Capture JS exceptions on the native (Obj-C/Java) side of the fence (and not using raven-js at all)
   • Upsides:
     • Behaves just like RN does today
     • The same plugin could capture both native and JS exceptions; no need to set up both
   • Downside is that this approach is complicated

IMO the 3rd approach is ideal, but I have no idea how much work it would be

[nevir]

I think I've just argued myself into hiding the PR behind a flag, at the very least - but I'm going to let it sit until the right approach is settled on

[benvinegar]

The 3rd approach involves using a different plugin, e.g. getsentry/raven-objc.

I think it's nice to be able to use Raven.js if possible, especially for cases where you're using the same React code on the web / on mobile devices. So I'm going to propose exploring #2.

cc @qbig @grabbou @dcramer

[nevir]

The thing that worries me about option 2 is that it's a bit deceiving for people who feel strongly about crashing hard.

The usual rationale for doing that (AFAIK) is to avoid letting the app get into a broken state, and especially to avoid it persisting any of that broken state. Waiting for the exception to send (or persist - offline support) gives the application plenty of time to do something bad

[benvinegar]

Total shot in the dark – I wonder if it's possible to do some kind of synchronous processing inside Raven.js while the HTTP request is being transmitted (e.g. synchronous XHR), so that control flow doesn't return to React Native. But I'm guessing that React Native doesn't actually support synchronous XHR.

[corbt]

I do think it's important for both minimizing possible damage and providing the best possible user experience to let the JS crash and RN restart as soon as possible after a crash happens.

Option 3 seems like a solid way to accomplish that, and also presumably would make it easier to integrate sentry into an app as well (because you only have to add code in one place, on the native side).

Another alternative would be to push the error to AsyncStorage instead of over the network and then crash, and only send it once the app starts back up.

[benvinegar]

Another alternative would be to push the error to AsyncStorage instead of over the network and then crash, and only send it once the app starts back up.

I'm in favor of this.

[nevir]

AsyncStorage doesn't block - and in typical use on iOS it adds 200ms
latency to any write

On Mon, Mar 28, 2016, 16:55 Ben Vinegar [email protected] wrote:

Another alternative would be to push the error to AsyncStorage instead of
over the network and then crash, and only send it once the app starts back
up.

I'm in favor of this.

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#533 (comment)

[nevir]

In favor of that over hitting the network, though!

On Tue, Mar 29, 2016, 09:45 Ian MacLeod [email protected] wrote:

AsyncStorage doesn't block - and in typical use on iOS it adds 200ms
latency to any write

On Mon, Mar 28, 2016, 16:55 Ben Vinegar [email protected] wrote:

Another alternative would be to push the error to AsyncStorage instead of
over the network and then crash, and only send it once the app starts back
up.

I'm in favor of this.

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#533 (comment)

[benvinegar]

What are the odds that writing to AsyncStorage has the same problems as firing an HTTP request? i.e. that the process terminates before the write occurs

[grabbou]

AsyncStorage doesn't block - and in typical use on iOS it adds 200ms

So far in production the number of exceptions reported that way equals the number of exceptions reported by Sentry native. I might be just lucky though, not relying on that too much since I also have native handlers as a fallback.

Downside is that this approach is complicated

I don't think it's complicated - it just requires quite a few additions to your native code, but AFAIK that's the most stable way to do it. If we decide to bundle native code for React Native, we could do a blocking write on native side and crash after that happens which should not produce any issues later (you can e.g. write to NSUserDefaults and read that from Javascript with Settings module or just add in your own methods)

think it's nice to be able to use Raven.js if possible, especially for cases where you're using the same React code on the web / on mobile devices. So I'm going to propose exploring #2.

Besides that, what I like in JS approach is you can send source maps and get nice stack traces for free.

I am giving my +1 to this #533 (comment) - I think it should cover most cases, the code is pretty portable and write/read is like 15 lines of code.

[grabbou]

One thing I don't understand in this PR (please enlighten me) is -> if you overwrite error handler and do not call it in production - app is never going to quit because of React Native JS exception. If that's something major, how you are supposed to reload the app then? (actually I just came up with an idea to reload the app just like refreshing browser w/o quitting!)

[nevir]

What are the odds that writing to AsyncStorage has the same problems as firing an HTTP request? i.e. that the process terminates before the write occurs

Would need some research on whether FB waits for any pending writes before crashing in earnest (I doubt it, though)

[abc123s]

1. Call the native handler after sending the exception, or timing out

In case it's helpful to others, here's an attempt at approach 2 described above. Despite the risk of waiting <1 sec before crashing, it made more sense for my use case; a user may not immediately re-open the app (or may just decide to uninstall!) in the event of a crash.

https://gist.github.com/abc123s/2d4bef879f79ff77b54703085bb077b0

[nevir]

Alright, I'm looking at tackling option 2 + AsyncStorage today. Here's my rough plan:

• Refactor Raven to expose:
  • Raven.snapshotException: Builds the entire payload that should be sent to Sentry (e.g. including breadcrumbs)
  • Raven.sendSnapshot: Sends the snapshot
• When a global error occurs:
  1. Snapshot the error
  2. Write the snapshot to AsyncStorage
  3. Rethrow the error
• When the plugin initializes:
  1. Read the snapshot from AsyncStorage (if it exists)
  2. call Raven.sendSnapshot
  3. Remove the snapshot from AsyncStorage

This should hopefully make #279 a lot more straightforward, too

@benvinegar I'm noticing that the current payload does not seem to include a timestamp for the error; though it does for the breadcrumbs. The API does support a timestamp though, right? (I'm assuming it just wasn't necessary in raven-js yet?)

[nevir]

Alternatively, this could be implemented as a transport (that passes through to the original one?). I would need to think a bit more about how that would be structured, though

[benvinegar]

(I'm assuming it just wasn't necessary in raven-js yet?)

Yeah, it will just use the creation time when received on the server end. But in this case it makes sense to just start collecting it (and more accurate). I think we may also have issues where breadcrumb timestamps don't match this event creation time because of clock skew issues.

[nevir]

👍

[benvinegar]

Okay, so just had a discussion with @mattrobenolt about this. We made this change because clients often have clocks that are just ... wildly inaccurate. Since we don't have control over that, the creation time is used.

I think in the case of React Native, where there's more control over the client (?), and because you're restoring from a previous event, it makes sense to send a timestamp. But I'm not sure if we should change the default behavior without more thought.

[nevir]

But I'm not sure if we should change the default behavior without more thought.

Sounds good - I'll bake it in from the plugin side only

[nevir]

Does the general approach of Raven.snapshotException -> Raven.sendSnapshot sound good to you?

[benvinegar]

So, I'm always super hesitant to add more API methods, and I'd like to avoid it as much as possible. Given that, I think you could get away with just adding a single sendPayload method.

Basically, you use shouldSendCallback to capture the payload and cancel sending (always return false). Then when you restore, use the newly minted sendPayload method to transmit the data.

It's a little hacky, but it lets us prove this out a little more without committing to a bigger API.

Thoughts?

cc @mattrobenolt

[nevir]

Yeah, sounds good to me; less surface area for me to potentially screw up.

Thanks for the feedback! Saved me a bunch of potentially wasted time/effort :)

[nevir]

Closing in favor of #626