"fatal" errors are not reported to the ExecutionContext

[yanns]

(follow up of #12150)

reproduction steps

using Scala 2.13.3,

the following scala test:

  "a fatal error in future" must {
    "be reported to the execution context" in {
      val parentEc = new ThreadPoolExecutor(1, 10, 100, TimeUnit.MILLISECONDS, new SynchronousQueue())
      val reportedError = new AtomicReference[Throwable]()

      val executionContext = new ExecutorServiceDelegate with ExecutionContextExecutorService {
        override def executor: ExecutorService = parentEc
        override def reportFailure(cause: Throwable): Unit = reportedError.set(cause)
      }

      val futureFailingWithFatalError =
        Future.unit.map(_ => throw new NoSuchMethodError("test"))(executionContext)

      a [TimeoutException] must be thrownBy {
        Await.result(futureFailingWithFatalError, 100.millis)
      }

      reportedError.get() must not be (null)
    }
  }

is failing with:

[info] - must be reported to the execution context *** FAILED *** (128 milliseconds)
[info]   The reference was null (ExecutionSpec.scala:143)
[info]   org.scalatest.exceptions.TestFailedException:

problem

The NoSuchMethodError should be reported to the executionContext 

[SethTisue]

@yanns could you update the example to be compilable as-is standalone? it's missing a lot of imports, and you're using several libraries you haven't specified the names and versions of, so that makes unneeded work for anyone who wants to try this themselves

[SethTisue]

@yanns have you searched for, read, and understood previous tickets in this area? there seem to be several, e.g. #9554. how confident are you that this isn't either 1) a duplicate, or 2) actually expected behavior?

[yanns]

Here is a standalone version:

import java.util.concurrent.atomic.AtomicReference
import java.util.concurrent.{ExecutorService, SynchronousQueue, ThreadPoolExecutor, TimeUnit}

import akka.dispatch.ExecutorServiceDelegate

import scala.concurrent.{Await, ExecutionContextExecutorService, Future}
import scala.concurrent.duration._
import scala.util.Try

object Main extends App {

  val parentEc = new ThreadPoolExecutor(1, 10, 100, TimeUnit.MILLISECONDS, new SynchronousQueue())
  val reportedError = new AtomicReference[Throwable]()

  val executionContext = new ExecutorServiceDelegate with ExecutionContextExecutorService {
    override def executor: ExecutorService = parentEc
    override def reportFailure(cause: Throwable): Unit = reportedError.set(cause)
  }

  val futureFailingWithFatalError =
    Future.unit.map(_ => throw new NoSuchMethodError("test"))(executionContext)

  val result = Try(Await.result(futureFailingWithFatalError, 100.millis))
  assert(result.isFailure)

  // should not be null, but be a NoSuchMethodError("test")
  assert(reportedError.get() == null)
//  assert(reportedError.get().asInstanceOf[NoSuchMethodError].getMessage == "test")

  executionContext.shutdown()
}

[yanns]

@yanns have you searched for, read, and understood previous tickets in this area? there seem to be several, e.g. #9554. how confident are you that this isn't either 1) a duplicate, or 2) actually expected behavior?

I don't feel so, but I'm not completely sure about it.
In general, it feels strange that we can have exceptions we cannot react on it.

[SethTisue]

@viktorklang at #9554 you wrote:

Fatal errors should also be rethrown such that they either surface in the Executor(Service) and can be dealt with there. Keep in mind that all execution after a Fatal Error is undefined.

I'd more than welcome some feedback on the new Future handling here.

is @yanns's example functioning as expected? cc @NthPortal

[viktorklang]

@SethTisue Fatal throwables are not reported, because after a fatal error execution is undefined and calling more code after observing one might obscure the "true" fatal exception. So that is bubbled up instead of reported-and-continue-as-usual: https://github.com/scala/scala/blob/2.13.x/src/library/scala/concurrent/impl/Promise.scala#L404

[NthPortal]

I thought it was reported (and apparently past me thought that as well: #9554 (comment)), but perhaps I was mistaken? I do feel it ought to be surfaced somehow, and not just silently swallowed. I know Akka's ExecutionContext handles it.

[NthPortal]

@viktorklang does that mean it gets handled by whatever UncaughtExceptionHandler?

[SethTisue]

@viktorklang happy to close the ticket, but I would like to understand what the upshot for the user is. is the upshot to use an UncaughtExceptionHandler somewhere...?

@NthPortal jinx :-)

[yanns]

How is it possible for example to stop the JVM is case of fatal exceptions?
Right now:

• the Future never completes.
• the reportFailure is never called.

From the application perspective, I'm missing a way to react to such exceptions and do something useful, like shutting down the JVM.

[yanns]

I see that I can use Thread.setDefaultUncaughtExceptionHandler. I'll try that.

[yanns]

I'm trying the following but it does not work:

import java.lang.Thread.UncaughtExceptionHandler
import java.util.concurrent.atomic.AtomicReference
import java.util.concurrent.{ExecutorService, SynchronousQueue, ThreadPoolExecutor, TimeUnit}

import akka.dispatch.ExecutorServiceDelegate

import scala.concurrent.{Await, ExecutionContextExecutorService, Future}
import scala.concurrent.duration._
import scala.util.Try

object Main extends App {

  val reportedError = new AtomicReference[Throwable]()

  Thread.setDefaultUncaughtExceptionHandler(new UncaughtExceptionHandler {
    override def uncaughtException(t: Thread, e: Throwable): Unit = {
      reportedError.set(e)
    }
  })

  val parentEc = new ThreadPoolExecutor(1, 10, 100, TimeUnit.MILLISECONDS, new SynchronousQueue())

  val executionContext = new ExecutorServiceDelegate with ExecutionContextExecutorService {
    override def executor: ExecutorService = parentEc
    override def reportFailure(cause: Throwable): Unit = {
      reportedError.set(cause)
      throw cause
    }
  }

  val futureFailingWithFatalError =
    Future.unit.map(_ => throw new NoSuchMethodError("test"))(executionContext)

  val result = Try(Await.result(futureFailingWithFatalError, 100.millis))
  assert(result.isFailure)

  // should not be null, but be a NoSuchMethodError("test")
  assert(reportedError.get() != null)
  assert(reportedError.get().asInstanceOf[NoSuchMethodError].getMessage == "test")

  executionContext.shutdown()
}

[NthPortal]

based on what Akka does (https://github.com/akka/akka/blob/a61b4868e06fd8a31a66b3898f75baa24d3a946e/akka-actor/src/main/scala/akka/actor/ActorSystem.scala#L816-L839), I am going to say with more confidence that an UncaughtExceptionHandler is the solution.

[viktorklang]

By default Global uses the default reporter as the UEH: https://github.com/scala/scala/blob/2.13.x/src/library/scala/concurrent/impl/ExecutionContextImpl.scala#L99 so yes, UncaughtExceptionHandler is the way to go.

[yanns]

To be honest, I'm not completely satisfied with the proposed solution.

Let me explain the context: we have servers handling lot of requests.
And for a very small portion of them, we got a NoSuchMethodError (that we fixed in the meantime).

For those requests, as the Future never completes, we cannot send any http response, and the client will timeout at some point.
But it would be much better for us to return a quick 500 error. It's more explicit for the users. It's more explicit that the application has an issue.

For the proposed solution, we can stop the server when such issues happen.
Not to break other clients, we have to gracefully stop the server, waiting for the in-fly-requests to be completed.
It means that clients will continue having timeout errors instead of explicit 500 errors.

Having a Future.failed still seem a better approach in that case.
But I guess it's a special context.

[viktorklang]

@yanns Typically NoSuchMethodError indicates that the classpath is broken—and as such the most appropriate default way of handling that is to exit the process as fast as possible. If you have cases where this is a normal situation, there's nothing which prevents you from adding a regular try { … } catch { case nsme: NoSuchMethodError => throw new DomainSpecificException(…, nsme) }

[yanns]

@yanns Typically NoSuchMethodError indicates that the classpath is broken—and as such the most appropriate default way of handling that is to exit the process as fast as possible. If you have cases where this is a normal situation, there's nothing which prevents you from adding a regular try { … } catch { case nsme: NoSuchMethodError => throw new DomainSpecificException(…, nsme) }

I've tried that approach, but it is not possible to put a try / catch everywhere. Our main abstraction is Future, which does not help here.

I've tried to put that in the execution context, but it also does not work:

      val executionContext = new ExecutorServiceDelegate with ExecutionContextExecutorService {
        override def executor: ExecutorService = parentEc

        override def execute(command: Runnable): Unit = {
          super.execute(new Runnable {
            override def run(): Unit =
              try {
                command.run()
              } catch {
                case e: NoSuchMethodError =>
                  throw new Exception(e)
              }
          })
        }

        override def reportFailure(cause: Throwable): Unit = reportedError.set(cause)
      }

My preferred option now is to add monitoring on UncaughtExceptionHandler.
Thanks anyway for taking the time.

[viktorklang]

@yanns

I've tried that approach, but it is not possible to put a try / catch everywhere.

Putting it everywhere would defeat the purpose—you should put it in the places where getting a NSME is a system-reasonable place to put it (or rather, where you allow it to occur). Catching it everywhere means that you could get NSMEs for reasons which are not safe to catch. So I think the appropriate to do is to analyze when/where NSMEs are to be expected in your codebase and put the try-catches there, while documenting why this is sound at that place.

Wrapping the Runnable won't work, as you discovered, since things are already "unrecoverable" at that point.