How should MobX handle errors in computed values and reactions?

[mweststrate]

As a follow up of #721 and several past issues where people found the logging about errors by MobX confusing. I played a bit with possible error handling strategies, but it is hard to find a best solution, but here are some possibilities:

Some facts up front:

1. Rethrowing an exception will cause the debugger to pause on the rethrow (especially if "pause on caught exceptions" is not ticked).
2. Logging an error (at least in Chrome) will always show the stacktrace of the log statement, and not of the exception!

How MobX could handle exceptions:

Solution 1. Never catch exceptions, make sure no state get corrupts by means of try / finally.

Print warning about the uncaught exception.

1. Pro: No issues with fact 1
2. Pro: No issues with fact 2.
3. Con: exception might be silently eaten in userland (leaving only the warning).
4. Con: Cannot log the exception (what started this issue).
5. Con: If a reaction throws, other pending reactions / computed values might not run anymore (until a next change), if if they maybe can, as the error could be unrelated. So more stuff then strictly necessary might freeze

This is the current behavior in MobX 2. Making the log message more clear could already improve things. Current:

mobx.js:969 [mobx] An uncaught exception occurred while calculating your computed value, autorun or transformer. Or inside the render() method of an observer based React component. These functions should never throw exceptions as MobX will not always be able to recover from them. Please fix the error reported after this message or enable 'Pause on (caught) exceptions' in your debugger to find the root cause. In: '[email protected]'. For more details see https://github.com/mobxjs/mobx/issues/462

Better?

An exception was thrown by a computed value, reaction or observer. Throwing exceptions might lead to unexpected results, please avoid throwing exceptions from derivations. In: '[email protected]'.

Solution 2: Consider exceptions a user problem

Don't try to recover from exceptions at all. After exceptions an app (or at least MobX) is just dead. Simple and efficient. Throwing is forbidden. No issues with fact 1 or 2, exceptions could be accidentally eaten in userland

Solution 3: Don't let exceptions escape

Catch all exceptions produced by reactions and produced values. Log when they happen

1. Pro: Simplifies implementation of MobX
2. Pro: Unrelated reactions are always run
3. Con: Exceptions might go by easily unnoticed as they are only logged
4. Con: Logging gives wrong stack mostly (see 2)

Solution 4: Save and rethrow exceptions

Catch all exceptions (like in 3) and rethrow them when either: 1. when the value of a computed value is requested (.get()), 2. or after all pending reactions have run

1. Pro: Also simplifies implementation of MobX a bit (hardly exception handling needed)
2. Pro: MobX can always run 'unrelated' reactions
3. Pro: Exceptions are caught and can be logged
4. Con: Debugger will break at the rethrow (unless 'break on caught' is ticked), but stack is correct
5. Con: Exception might still be eaten in userland. At least log about the rethrow?

Solution 5: Save and rethrow exceptions on separate stack

Same as 4, but without downside 4. Throw the saved exceptions from a new stack using setImmediate, makes it impossible to 'eat' the exception, and impossible to actually catch the exception in userland

---

Note that running everything in sync is usally an advantage as people could catch exceptions in userland, although sometimes it leads by unwary users to problems, e.g.

fetch(data).then(json => {
   try {
      someObservable = JSON.parse(json)
   } catch (e) {
      // ignore JSON exceptions
      // .... but this also eats exceptions thrown by derivations that are run as a result of assigning someObservable!
   }
})

@benjamingr did you give exceptions that have no handler special treatment in bluebird? If I remember when library did go the extra mile by trying to detect this?

[benjamingr]

I think this: tc39/proposal-observable#119 brings up a lot of interesting points about the issue. Namely how throwing the exception when it happens prevent the other observers to run and breaks the illusion that an observer can ignore the fact the observable is multicast.

I think a reasonable alternative to option # 1 is to call HostReportErrors directly (dispathcing a window error event) but still running other observers. I'd definitely expect my app to mostly keep functioning in production if one edge case in one page I never visit throws an error because something changed in an uncoordinated way - so I'm strongly against anything that implies the app stops functioning at this point.

---

@benjamingr did you give exceptions that have no handler special treatment in bluebird? If I remember when library did go the extra mile by trying to detect this?

Bluebird stitched stack traces manually across promise contexts in order to give the user a real stack trace. Since in MobX the whole stack trace is available synchronously it is not necessary here. In addition bluebird logs by default on unhandled rejection (like NodeJS and native promises today - but bluebird did it 2 years before).

[spion]

Are you sure about Con 4 of Solution 3? I don't see why logging would affect the original stack trace...

If the trace is correct, you can use it to set breakpoints and debug so I think solution 3 would make the most sense.

[mweststrate]

@benjamingr thanks, great link!

@spion try throwing a new Error and catch it up in the stack and log it using console.error or friend; the trace you see is from the log statement, not from the error (at least in chrome)

[amir-arad]

There is a con not mentioned above, regarding try-catch and try-finally: both are optimization killers.

Solution 2.5 Ignore user exceptions, but keep mobx consistent

register a setImmediate cleanup before each reaction loop and cancel it after the loop completes.
this cleanup can start by logging that mobx is recovering from a user error in reaction so-and-so, and resume the reaction loop.

[amir-arad]

an optional spy API that catches the actual error object and attaches it to the error event can be a huge benefit as well.
such a solution avoids the logging stacktrace problem while providing valuable informations for tests, debuggers etc.

[mweststrate]

@amir-arad yep that is a good option, especially with a custom throw hook. I did some measuring in mobx with try / finally in the past, but the performance impact of try / catches was neglectable, so I am primarily concerned about the best solution, and less about the fastest one. If throwing is done async, a dedicated hook would be great indeed!

[danielearwicker]

#4 was what I came to suggest! Exceptions ripple up the stack through layers of computeds. Reminds me of Task.Result behaviour in .NET.

[spion]

@mweststrate as far as I can tell, the stack is complete: https://jsfiddle.net/wejkk0vj/

In some browsers (FF/Chrome) If you expand the logged line however, the items will indeed be missing. But the original error stack is also present... Not sure what happens in Edge?

[amir-arad]

@spion I think @mweststrate refers to the link in the right side of the log message, and the expandable native stack trace below it, that points to the source of the console.log action (in chrome).
Below image is taken from the result of running your jsFiddle:

[spion]

Right, but the original stacktrace is still there - so I don't see how the log source is an issue...

[mweststrate]

@spion @amir-arad I think this screenshot explains it the best (chrome 54)

Note that only catcher is shown in the log, and when you click it you will end up in the console error. There is no mention of either callThrowingFunc or throwingFunc, while the actual interesting stack is ThrowingFunc > callThrowingFunc > catcher, instead of just catcher. I hope that makes the issue more clear! Debugging this will lead the developer initially to the mobx catching / logging code, not to the original exception in userland, nor is there any clear hint of that

[benjamingr]

@mweststrate huh?

[benjamingr]

JavaScript stack traces are determined when the error is created, not when it is last thrown.

[andykog]

@mweststrate, you have to click (...) and then click on the stack trace again :-)

[spion]

@mweststrate hmm thats quite strange. wonder what option in chrome causes the difference in display, and which one is the default.

[mweststrate]

haha I feel such a noob right now 🐰 @andykog thanks! @spion sorry for the confusion! @benjamingr yeah the stack is correct if you inspect error.cause, I just couldn't find it anywhere when logging errors. 😢

[mweststrate]

At this I think 4. is the most ideal solution. Catch all and throw async is probably a nice plan B, but I don't like it conceptually that in MobX, where everything runs sync, the exceptions would be thrown async. This could lead to buggy scenarios where an exception, caused by the original error, is thrown before the original error.

Implementation wise 4. is quite challenging, not sure yet whether it is doable. The basic idea is in pseudo code

let storedException;

observable.set = function(x) {
   this.value = x
   this.propagateChangesRunDerivationsEtc()
   if (storedException)
      throw storedException
}

function propagateChangesRunDerivationsEtc() {
   pendingReactions.forEach(reaciton => {
      try {
          reaction.run()
      } catch (e) {
          if (!pendingException
              pendingException = e
      }
   }
}

I think this is similar to how it is done in RxJS, and the reasoning is that in all the mobx internal functions there is no need to deel with exceptions, they are 'transported' from the original throwing block to the outermost userland-invoked method invocation.

In practice it might be a bit more tricky, as there are also a lot of invariants in mobX functions that could be caused by the user, which might need proper handling as well. So I am not sure whether this should be / will make it to 3.0.0.

Let me know what you think of these plans!

@

[mweststrate]

In the mean time here is the poll outcome (sadly twitter didn't allow for a fifth option)

[andykog]

@mweststrate, the poll is formulated incorrectly, I see that only 36% of voters think that app should die on exception, other 64% think it should not.

[mweststrate]

@andykog good one. This didn't really fit in a tweet, as each answer is max 25 chars..

Anyway, thought about this more, actually the throw async isn't that bad, as errors should hardly propagate:

Currently there are two kinds of userland caused errors

1. errors in a computation
2. errors in a reaction (autorun, observe hooks etc are all variations of this)

Errors in (1.) can be caught easily and rethrown to the next consumer that .get()'s the value (maybe after re-evaluating). This includes distributing the change to downstream computed values (they are notified about the change, and then will run into the error when they get the error as result of .get()ing the original exception

Errors in (2.) can be safely caught so that they don't affect other running reactions. The errors could then be rethrown asynchronously.

For testing purposes, the spy "error" could still report synchronously, and a custom onErrorHandler could be established.

---

As an alternative solution to errors in (1.); computed values could take the same approach as (2.), and instead of rethrowing the error it could freeze the current value. The downside is that this 'freezing' could be not noticed (same holds for the reactions of course).

(These are quite drafty notes, so if it needs more elaboration just let me know)

[andykog]

@mweststrate why is asynchronous rethrow better then just console.error(error)? It seems that async errors can be harder to debug:

console.log('↓ May thow here:');
doTheStuff();
console.log('↑ Any errors?');
// ↓ May thow here:
// ↑ Any errors?
// ...
// Error!

[mweststrate]

@andy maybe both? Op ma 2 jan. 2017 22:18 schreef Andy Kogut <[email protected]>:

…

@mweststrate <https://github.com/mweststrate> why is asynchronous rethrow better then just console.error(error)? It seems that async errors can be harder to debug: console.log('↓ May thow here:');doTheStuff();console.log('↑ Any errors?');// ↓ May thow here:// ↑ Any errors?// ...// Error! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#731 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvGhJcr6ekBCk0visa1TsSKOUHIY78Wks5rOWmKgaJpZM4LYsmY> .

[andykog]

@mweststrate, i'm just not getting the purpose of that async rethrow, if you are concerning about debugger being stopped, it will still happen in random place (sources of mobx) and, in case of async rethrow, at random time, so it won't be helpful at all.
If we catching error, there is no way to make break on exception useful

[mweststrate]

The rethrow was also to fail more loudly, but i guess in practice it doesn't differ from console.error in the browser. And in now the process would die completely. So i think you are right, let's scratch that part of the proposal. Op ma 2 jan. 2017 22:33 schreef Andy Kogut <[email protected]>:

…

@mweststrate <https://github.com/mweststrate>, i'm just not getting the purpose of that async rethrow, if you are concerning about debugger being stopped, it will still happen in random place (sources of mobx) and, in case of async rethrow, at random time, so it won't be helpful at all. If we catching error, there is no way to make break on exception useful — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#731 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvGhI8e40IhFlkM-ItuWMszX4vrx7x2ks5rOW0XgaJpZM4LYsmY> .

[mweststrate]

Now = node

[mweststrate]

Implemented improved error handling in #736. Reviews / tests are appreciated!

[mweststrate]

Closed as the this is released as part of 3.0.0