catch exceptions thrown by handlers on dispatch

[hoodie]

this is a rebase of Dispatched signals should not throw

[codecov-io]

Codecov Report

Merging #26 into master will decrease coverage by 0.55%.
The diff coverage is 95.23%.

@@            Coverage Diff             @@
##           master      #26      +/-   ##
==========================================
- Coverage     100%   99.44%   -0.56%     
==========================================
  Files           5        5              
  Lines         151      179      +28     
  Branches       15       19       +4     
==========================================
+ Hits          151      178      +27     
- Misses          0        1       +1

Impacted Files	Coverage Δ
src/signal.ts	98.07% <95.23%> (-1.93%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ccc139d...de98f2b. Read the comment docs.

[jmaher409]

I think it might make sense to combine this with the RFC PR you made so that we also add a way to observe the exceptions.

[hoodie]

I'd like to at least have this safety net. The other one is a feature, where this is more of a fix. Could we have this as a minimum first step and then work on the other?

[lelandmiller]

I still stand by my last comment in #18 (comment). This change makes me really uncomfortable. I am alright if we find a way to run through the rest of the listeners, but I do not think we should blindly swallow all exceptions.

Personally, I think that an uncaught exception should really crash everything, but not even logging it for developers to see seems really scary. I could imagine cases where an integration issue that isn't caught by unit tests could cause even simple exceptions getting missed by a developer and causing unexpected behavior that would then be really difficult to debug (since there are not even logs or anything). Does that make sense?

Sure, from an individual library perspective, we don't want the dispatch to throw an exception due to a listener in another library's listener, but from the whole app (and the user) perspective we definitely want to know. A failure in a listener will almost always represent a failure within a module, or a failure in another module that is intentionally integrating with the dispatching module. In both of these cases it seems that we need to know about any exceptions, and even if there are exceptions that can be ignored, that they should be ignored on a case by case basis.

For a theoretical example, imagine a signal that is dispatched when a user would like to change a mute state for example. Though a failure in a listener doesn't represent a failure in the UI code, in the context of the application it does represent a failure to change the mute state, and the stack trace ending at that dispatch can actually be really helpful for tracing the issue. In addition, imagine a situation where the application was implemented with two separate listeners on that mute state signal, one to update the UI and one to mute the microphone. In this case, swallowing a failure could mean that the microphone was never actually muted, but the UI was updated to show that the microphone is muted and the failure was never logged or reported. Though this wasn't the UI's fault, I would definitely want to know as an application that integrated the UI library and the audio library.

There is also the whole case where Signals are used dispatched and listened to within a library, in which case I would think we would always have the information to debug the failure.

Though the dispatch point may not see the exception as actionable as it does not understand the failure, there is a point within the application context at which the exception is actionable, even if it is at the top level by a developer who sees it in the logs. The appropriate thing to do with an exception like this is let it reach that point.

I personally feel like this change doesn't add a safety net, but removes the safety net of knowing when listeners have throw an exception. Even in less contrived failures than the mute button one, it still seems like swallowing exceptions opens all consumers of this library up to strange edge cases that are very difficult to debug since exceptions are being swallowed.

I could have the wrong perspective around this, so please let me know your opinion! 🙂

[hoodie]

Hi Leland! Thanks for the elaborate discussion. I'm sorry I had to be brought up to speed this way. Of course this would allow us to oversee something that we add ourselves during dev-time and on that regard, I'm completely on the same page. We came from being crashed by some odd listener somewhere in a much higher abstraction layer that we had no control over or even insight into and that actually tore down entire sessions this way. I'd love to find a middle ground here.

My original idea was to add something explicit like a CatchingSignal (no idea what that would look like) or at .dispatchSafe(), but I was convinced along the way that no signal should throw ever. Thinking about it, that's not correct. The other PR I opened seemed a bit too much to myself at some point too.

What do you think about these alternatives?

A: Catch everything all the time
B: Have a special Signal that catches all the time
C: Have a special Signal that allows these catchers as described in #27
D: Have a special dipatch for when you don't trust your listeners

[jmaher409]

I personally like option C as it allows users to option to doing the right thing and covers our bases regardless. I imagine usage looking like:

signal.catch((e) => console.error(e));

as a minimal/naive implementation. I could also see this mixing nicely with the default listener feature such that we could add a setDefaultCatchListener static method that calls some default code whenever a listener throws.

Edit:

I hadn't seen Leland's comment above. I actually quite like the idea of always throwing when any listener throws and causing the whole app to crash. That way it works like any other uncaught exception in js.

[hoodie]

proposal E: add a catch callback right to the dispatch call:

    public dispatch(payload: T, catchWith?: (e: any) => void): void {
        if (this._listeners.size === 0) {
            const instanceDefaultListener = this._instanceDefaultListener;
            instanceDefaultListener(payload);
            return;
        }
        if (catchWith) {
            this._listeners.forEach(listener => {
                try {
                    listener.call(undefined, payload);
                } catch (error) {
                    catchWith(error);
                }
            });
        } else {
            this._listeners.forEach(listener => {
                listener.call(undefined, payload);
            });
        }
    }

this has the following advantages:

1. no breaking change, unhandled dispatches still throw
2. allows to inject own catching behaviour without overhead (no extra listener management etc)
3. it allows creating a derived CatchingSignal really easily by having that one just forward dispatch with a special catching function

[lelandmiller]

Either way, proposal C as described by Justin (signal.catch((e) => console.error(e));) or proposal E seem alright to me. In both of these, we opt into handling exceptions, and throw otherwise.

Proposal C is nice because it fits in with the existing Signal semantics. Proposal E is nice because it really exposes the minimal behavior required to appropriately handle exceptions within the listener set and allows users to build onto that. Proposal E allows more granularity to exception handling, but proposal C might be the most common use case anyway (and if really desired, a user could merge multiple #catch created signals into one to provide more granularity to a single exposed signal).

Both of these proposals are reasonable to me, and I would definitely consider a PR around either 🙂

[hoodie]

I implemented both C and E. Now you can pick from either of these options:

1: signal.dispatch(42); // will just throw
2: signal.dispatch(42, e => {...}); // catches in place
3: let catching = signal.catch(e => {...}); // creates a catching signal
   catching.dispatch(42);

[jmaher409]

I'm kind of leaning toward having both C, E AND having a static exception handler configurable for all signals. This could look something like:

// creates a new Signal constructor that is preconfigured
const MySignal = Signal.configure({ exceptionHandler: e => {
    console.error('staticHandler' + e);
} });
const signal = new MySignal();
signal.add(p => throw new Error('foo'));
signal.dispatch(42); // Now this is caught by static handler configured for MySignal
signal.dispatch(42, e => {...}); // catches in place
let catching = signal.catch(e => {...}); // creates a catching signal
   catching.dispatch(42);

This makes it so we can introduce some defensive code for production immediately across the code base and use the more fine grained ones for specific error handling.

[hoodie]

feel like just adding that to the PR?

[jmaher409]

I can :).