ExecutionContext.defaultReporter should try to use Thread.getDefaultUncaughtExceptionHandler if available

[dylemma]

From https://github.com/scala/scala/blob/2.13.x/src/library/scala/concurrent/ExecutionContext.scala#L221 I see that ExecutionContext's defaultReporter just does _.printStackTrace.

I found that the Monix library has a similar mechanism for its Scheduler concept, where the default error reporter tries to look up Thread.getDefaultUncaughtExceptionHandler before falling back to the old printStackTrace approach. See https://github.com/monix/monix/blob/v3.0.0-RC2/monix-execution/jvm/src/main/scala/monix/execution/internal/DefaultUncaughtExceptionReporter.scala#L28

I think this is desirable behavior for all in-JVM cases, since it allows you to set up logging or other custom error-handling side effects without having to resort to creating a custom ExecutionContext.

[SethTisue]

@viktorklang ?

[viktorklang]

"Interface for handlers invoked when a Thread abruptly terminates due to an uncaught exception.

When a thread is about to terminate due to an uncaught exception the Java Virtual Machine will query the thread for its UncaughtExceptionHandler using Thread.getUncaughtExceptionHandler() and will invoke the handler's uncaughtException method, passing the thread and the exception as arguments. If a thread has not had its UncaughtExceptionHandler explicitly set, then its ThreadGroup object acts as its UncaughtExceptionHandler. If the ThreadGroup object has no special requirements for dealing with the exception, it can forward the invocation to the default uncaught exception handler."

• https://docs.oracle.com/javase/8/docs/api/java/lang/Thread.UncaughtExceptionHandler.html

Given the description above I am not certain (at all) that it is the correct thing to do as a general default.

[dylemma]

I think something should be done to make the default "uncaught exception" behavior more configurable.

Consider the following scenario:

import scala.concurrent.ExecutionContext.Implicits.global

val f = Future {
  // let's say the program had some issues, and OOM'd
  throw new OutOfMemoryError
}
f.onComplete { /* notify the user of success or failure * }

In this, the OutOfMemoryError gets printed to StdErr, but f never completes. Depending on your application, you might be able to recover from a situation like that (killing the thread that OOM'd might recover enough memory to resume execution), or you might not (now you have a future that will never complete, so your program is in an undefined state, so you'd rather crash).

As far as I can tell, if you don't explicitly set the uncaughtExceptionHandler, java will call printStackTrace for you anyway:

val executor = Executors.newSingleThreadExecutor(new ThreadFactory {              
  def newThread(runnable: Runnable): Thread = {
    val t = new Thread(runnable)               
    t.setDaemon(true)                      
    t                                          
  }                                            
})    
executor.execute(new Runnable {
  def run() = throw new OutOfMemoryError
})
// stack trace gets printed

// and if you set an explicit handler...
Thread.setDefaultUncaughtExceptionHandler(new Thread.UncaughtExceptionHandler {
  def uncaughtException(t: Thread, e: Throwable) = {                           
    println(s"Oh no an error in [${t.getName}]!!")   
    e.printStackTrace()                                                        
  }                                                                            
})
executor.execute(new Runnable {
  def run() = throw new OutOfMemoryError
})
// prints Oh no an error in [Thread-3]!!
// and then prints the stack trace

Because of this behavior, I'd argue that the default ExecutionContext should either pass nothing (letting that default behavior happen on its own, so that a global UncaughtExceptionHandler can still apply), or pass something more explicit that is configurable. Akka seems like a good example

Also, maybe I'm interpreting the javadocs differently but given

If the ThreadGroup object has no special requirements for dealing with the exception, it can forward the invocation to the default uncaught exception handler

It seems like Scala has no special requirements for dealing with the exception (given the printStackTrace approach), so it should forward the invocation to the default exception handler.

[viktorklang]

@dylemma I think my current view on it is that reportError(e) is not equivalent in semantics to ueh.uncaughtException(t, e), for the simple reason that reportError is used whenever there is no natural channel to forward an error to. Changing the defaultReporter will have effect on a lot of codebases as I think it is rare that users specify their own. So if we are to change the default semantics of it—we need to be convinced that it is a beneficial change that doesn't cause unintended runtime issues.

[dylemma]

reportError(e) is not equivalent in semantics to ueh.uncaughtException(t, e), for the simple reason that reportError is used whenever there is no natural channel to forward an error to

That may be the intention, but the reality is that the reporter function is being used for both the functionality of the reportFailure method on ExecutionContext, and the UncaughtExceptionHandler in the underlying thread pool (source). From this I'd argue that either they are semantically the same, or this is a bug.

In my previous post I was trying to show that the defaultReporter's behavior is the same as the default UncaughtExceptionHandler behavior (which is to just call printStackTrace), and so explicitly passing _.printStackTrace as the UncaughtExceptionHandler is unnecessarily getting in the way of letting a user override that behavior themselves.

A suggestion:

Inside def createDefaultExecutorService(reporter: Throwable => Unit), replace the uncaught definition with:

uncaught = (thread: Thread, cause: Throwable) => {
  Thread.getDefaultUncaughtExceptionHandler match {
    case null =>
    case ueh => ueh.uncaughtException(thread, cause)
  }
  reporter(cause)
}

• In the vast majority of cases, this would result in no change in behavior, since the default UEH is null.
• It would still honor the intentions of the reportFailure method on ExecutionContext by calling reporter(cause).
• The beneficial change would be that for users who did set a default UEH, you would now be participating in their explicitly-chosen behavior instead of dodging it.

[viktorklang]

That may be the intention, but the reality is that the reporter function is being used for both the functionality of the reportFailure method on ExecutionContext, and the UncaughtExceptionHandler in the underlying thread pool (source). From this I'd argue that either they are semantically the same, or this is a bug.

That is for a specific implementation of ExecutionContext where both the EC interface and the ThreadPool implementation is known. You are talking about a generic change across all implementations which do not specify their own reporter.

For those who have specified their own UEH, they could also specify their own reporter right now, right?

[dylemma]

For those who have specified their own UEH, they could also specify their own reporter right now, right?

Admittedly, yes, but it turns out to be a pain if you were using the Implicits.global ExecutionContext:

• Create your own "global replacement" ExecutionContext by calling ExecutionContext.fromExecutor(null, customReporter)
• Replace all imports to the Implicits.global with an import to your replacement
• Figure out a way to ensure the Implicits.global never gets imported again. Automation? Code review? Still TBD/TODO in my case.

Side note:
One of my teammates suggested simply allowing the ExecutionContext.defaultReporter to be changed by the application, similar to how Thread.defaultUncaughtExceptionHandler can be changed. But I can see how that opens things up for abuse e.g. if a library decided to silently change the handler.

a generic change across all implementations which do not specify their own reporter

That's not my intent. I should clarify that with my most recent suggestion, the ExecutionContext.defaultReporter would remain untouched. You pointed out that I described a problem in a specific implementation of ExecutionContext, and my suggestion is intended to apply to exactly that case.

ExecutionContextImpl#createDefaultExecutorService is a private method that is only called from ExecutionContextImpl#fromExecutor and ExecutionContextImpl#fromExecutorService when you pass null for the Executor/ExecutorService argument (which is how the global EC gets created). And since my suggested implementation of the UEH still calls the reporter function regardless of the global-default UEH, the actual value or reporter (default or custom) should not be affected.

My intent is that the change will only apply to cases where both:

• the user is using an EC created from one of those two helper methods by passing null as the Executor/ExecutorService argument (and therefore letting scala provide the implementation)
• the user has also set a custom global-default UEH.

In this specific case, when a fatal exception is thrown from one of the EC's threads, the global UEH behavior should be called in addition to calling the EC reportFailure function. The actual reportFailure behavior in the EC should be unaffected. In all other cases, there should be absolutely no change in behavior.

[viktorklang]

I've been thinking about this over the weekend and I'm just going to try to summarize the current state:

• The global EC and all ECs created using the same method currently get an UnhandledExceptionHandler installed via the ExecutionContextImpl.DefaultThreadFactory which always is (thread: Thread, cause: Throwable) => reporter(cause)—the reportError method of the resulting EC then forwards exceptions to that UEH.

This narrows the problem down to the following: "How do I intercept reportError för the global EC?"
The answer could be: "What would you do there?"

I've been wanting/planning to use ForkJoinPool.commonPool() as the backing for EC.global for quite some time, and the common pool supports configuring your own UEH via a System.property so the question is whether it makes sense to either A) add a system property option to EC.global, or B) Use the ForkJoinPool.commonPool.getUncaughtExceptionHandler as a fallback as is, or C) recommend that people wrap EC.global and implement their own reportFailure method, or D) leave things as they currently are

Thoughts?

[dylemma]

I'm having trouble writing down a cohesive train of thought, so bear with me...

• Three things at play: EC's reportFailure method (I think that's what you were referring to as reportError), the ThreadFactory's UEH, and the underlying reporter function.
• As you described, reportFailure forwards exceptions to the UEH.
• And the UEH forwards exceptions to the reporter function.
• If a "fatal" exception happens on one of the FJP threads, the UEH will trigger and cause the reporter function (e.g. ExecutionContext.defaultReporter) to be called, but it won't call the EC instance's reportFailure method.
• If you switched to FJP.commonPool as the underlying thread pool for "default" ECs and didn't also inject your own UEH to that pool, "fatal" errors would never make it to the reporter/reportFailure parts of the EC.
• So I think that EC has to put its own UEH on its underlying thread pool... which I think would be a problem for end users who also want to put their own UEH on the FJP.commonPool

You mentioned switching over to FJP.commonPool as the backing EC.global thread pool, but I'm not sure if that also applies to your list of suggestions. I think (A) makes the most sense if you don't switch, since it would just be a matter of letting the user swap out the functionality of ExecutionContext.defaultReporter. I think (B) makes the most sense if you do switch, since it would be weird if you used FJP.commonPool's UEH without actually using its threads. But the train of thought I listed above makes me think that it won't be so simple.

For (B), if you don't switch to FJP.commonPool, that sounds the most like what I was shooting for in my earlier posts. If EC is still constructing its own thread pool, it can assign a UEH implementation that follows the recommendation from the UEH docs and also injects the reporter somewhere in there:

If a thread has not had its UncaughtExceptionHandler explicitly set, then its ThreadGroup object acts as its UncaughtExceptionHandler. If the ThreadGroup object has no special requirements for dealing with the exception, it can forward the invocation to the default uncaught exception handler.

So e.g.

• Check the thread pool's UEH (assuming it's not the one you are explicitly creating) and delegate to it
• If that was null, check the global UEH and delegate to it
• Finally, call the reporter function.

For (C), that introduces the pain of having to explain that people to ignore the helpful @implicitNotFound message from ExecutionContext that tells them to import the global one. And of course I'm not a fan of (D) or I wouldn't have opened the issue in the first place 😄

I think if we can find a way around the issues with using FJP.commonPool, that would be pretty cool.
Maybe rather than setting up a custom UEH, the Runnables that get submitted to the pool could call EC's reportFailure method. E.g.

new Runnable() {
  try {
    val result = doWork()
    callback(Success(result))
  } catch {
    case NonFatal(e) => callback(Failure(e))
    case e: Throwable => 
      ec.reportFailure(e)
      throw e
  }
}

so the "fatal" exceptions can get "reported" to the EC, and then rethrown so the UEH semantics can apply regardless of what thread pool you decide to use under the hood.

[viktorklang]

I'm not sure what you are proposing solves the problem—the UEH can't call reporter if the reportFailure calls the reporter, and the UEH can't call the reportFailure because it wouldn't know what reportFailure to call (in this case I guess it could look at the current Thread to figure out if it is a ForkJoinWorkerThread and if it's getPool also extends ExecutionContext, but that wouldn't work if any wrappers, since it would call the pool's reportFailure rather than the wrapper's reportFailure).

[leventov]

What about picking a special property which allows configuring a special reporter for ExecutionContext.global, similar to FJP's java.util.concurrent.ForkJoinPool.common.exceptionHandler property? It would not break backward compatibility, yet people who want could report to a logging library instead of printing the stack trace to System.err.

[viktorklang]

@leventov The simplest option might be to have global use FJP.commonPool's getUncaughtExceptionHandler, and fall back to System.err if it is null. Then you would et the same behavior for FJP.commonPool as for EC.global.