Catching errors thrown in change event callbacks leaves the `_changing` true and won't trigger any change events after

[2color]

var theModel = new Backbone.Model();

theModel.on('change:name', function(model, val) {
    if(val === 'Jesus') {
        throw new Error('The name is taken');
    }
});

theModel.on('change', function(model, val) {
    console.log('change', arguments);
});

theModel.set('name', 'Default Name'); //Triggers a change event

try {
    theModel.set( 'name', 'Jesus' );
} catch (e) {
    console.error(e);
    theModel.set( 'name', 'New Name'); // Won't trigger a `change` event. Only a `change:name` event.
}

The problem I'm experiencing requires me to manually set theModel._changing = false; theModel._pending = false;. Otherwise no change event will be triggered after.

[akre54]

Use validate for this.

[jridgewell]

While #validate is a better for for the example, I think that change events can't fire after an error in any change listener is a problem.

[akre54]

Why would a change event handler throw an error? Errors in JS are pretty bad, but they're almost universally reserved for exceptional (unexpected) behavior. Anything that you know ahead of time that can throw an error in your change handler you should wrap in a try-catch yourself.

[jridgewell]

I'm not saying that errors should be trivial, but for an error to prevent library code from functioning correctly seems like a problem.

[akre54]

But I can't see a case when you wouldn't want to wrap potentially error-throwing code in a try-catch within your change handler so that you could deal with it. Uncaught errors probably shouldn't mess up Backbone's internal state, but if you've thrown an uncaught error, your program is crashed. What difference does it make what happens in Backbone after that?

We can't wrap all userland code in a try-catch, that would be slow as hell.

[jridgewell]

if you've thrown an uncaught error, your program is crashed

Only for this turn of the event loop. This is the only place (I think) in Backbone that can break the next turn as well, because we're storing state to support nested change events.

We can't wrap all userland code in a try-catch

Definitely don't want to suggest this. I'm fine with an error breaking the app and it's on the end dev to catch potential errors. But it feels funny that an error can break Backbone's API on any following event loops.

We can't wrap all userland code in a try-catch, that would be slow as hell.

We can mitigate that with a wrapper function. It might not be appropriate for underscore, but can easily add it as a private function in Backbone.

function tryCatch(func) {
  try {
    func();
    return true;
  } catch (e) {
    return false;
  }
}

class Model {
  set(key, val options) {
    // ...
    var model = this;
    if (!silent) {
      if (changes.length) this._pending = options;
      var ok = tryCatch(function() {
        for (var i = 0; i < changes.length; i++) {
          model.trigger('change:' + changes[i], model, current[changes[i]], options);
        }
      });
    }

    // You might be wondering why there's a `while` loop here. Changes can
    // be recursively nested within `"change"` events.
    if (changing) return this;
    if (!silent && ok) {
      tryCatch(function() {
        while (this._pending) {
          options = model._pending;
          model._pending = false;
          model.trigger('change', model, options);
        }
      });
    }
    this._pending = false;
    this._changing = false;
    return this;
  }
}

[akre54]

This just wouldn't be sustainable. Potentially every parse, initialize and event listener could screw up Backbone's internal state. We can't wrap them all.

If your code throws an error, it's your responsibility to clean up after it.

I see where you're coming from though, @jridgewell, and if it were just change listeners that'd be a different story. (Valid point about _.attempt, I'd forgot about that de-opt fix). How's the perf look when applied to change events, and potentially complex change events?

[jridgewell]

This just wouldn't be sustainable. Potentially every parse, initialize and event listener could screw up Backbone's internal state.

I completely agree, we can't wrap everything. But this is the only situation where we can leave Backbone in an inconsistent state. If an initialize blows up, or a change:attr listener does, then the model's data could be inconsistent, and that's definitely a dev concern. If a change listener does, we can no longer trigger change events, and that seems like a library concern.

How's the perf look when applied to change events, and potentially complex change events?

http://jsperf.com/change-event-try-catch

[akre54]

That test isn't using Jbone. Try this.

On a more general note, not every edge case needs to be accounted for. We're not building nuclear testing software here. Sometimes the added code to handle these edge cases makes the rest of the code harder to reason about (see the latest events refactor, e.g.). Sometimes we have to say "you know that thing that's a bad idea? Don't do it. We aren't going to account for it". We shouldn't be responsible for wrapping faulty dev code.

[jridgewell]

That test isn't using Jbone. Try this.

Man, I knew something was fishy.

Sometimes we have to say "you know that thing that's a bad idea? Don't do it. We aren't going to account for it". We shouldn't be responsible for wrapping faulty dev code.

Works for me.

[disruptek]

The real problem is that we cannot know what the proper course of action is, because the state is already broken if the exception was not handled by user code. Any future code that executes does so with a broken contract.

As @akre54 alludes to (and as I've detailed), as long as the event system cannot be reasoned about, there's no way for the user to adjust any in-flight events even if they were to catch the exception.

The correct course is to fail, and fail hard. Any change to the current, correct behavior is (severely) breaking.

[akre54]

@disruptek :+1: