ThreadCallbacks discarding exceptions

[ncannasse]

I saw this code in ThreadCallbacks:

	static function iterateCallbacks<F>(callbacks:Array<ThreadCallback<F>>, f:ThreadCallback<F> -> Void) {
		var firstException = null;
		for (c in callbacks) {
			if (!c.isClosed) {
				try {
					f(c);
				} catch(e:Exception) {
					if (firstException == null) {
						firstException = e;
					}
				}
			}
		}
		if (firstException != null) {
			throw firstException;
		}
	}

I think it's a very bad idea for several reasons

• it prevents the debugger from breaking at the place you have an exception, you're instead breaking on the throw firstException
• it somehow discard all user exceptions happening after the first one, which is really a bad behavior from the user point of view.

In general, the whole "exception during abort or exit which triggers onAbort" is not very well thought through.

[Simn]

Callbacks raising exception is always a problem, but I'm not sure what you're actually suggesting we should change. The alternative is to not catch anything, which would mean callbacks are missed as soon as one of them throws.

[ncannasse]

@Simn I made a first fix for single callback, but it's only temporary.
In general I think that we shouldn't try to arbitrary resume user code in our system. If there's an uncaught error the role of the Thread toplevel is to report it and stop, not more. We can't give any guarantee about overall system stability when an exception happens, so ignoring some of them is even worse.

[Simn]

I can't really follow that argument because the observable exception doesn't change here. The only difference is that we (try to) execute subsequent callbacks before throwing the exception, which might ensure that important cleanup code runs even when a random callback throws.

For example, under EventLoop usage if you register an onExit callback that throws, code like events.dispose() no longer executes which will probably cause hard to detect leaks.

The debugger point makes some sense to me but I disagree on the "bad behavior" one. To me this just seems more robust than letting exceptions eagerly leak. However, if this is how everyone else handles this I'm not going to impose.

[ncannasse]

My point is that any exception in user code that is not handled correctly can trigger some potential hard to detect leak. I don't see why thread callbacks should make a difference here, and it seems weird that if two callbacks have exceptions, I only get the first one reported while the other exception is silently ignored.