scala / bug

Scala 2 bug reports only. Please, no questions — proper bug reports only.
https://scala-lang.org
230 stars 21 forks source link

Future does not document behavior on fatal errors #12779

Closed som-snytt closed 1 year ago

som-snytt commented 1 year ago

Reproduction steps

Scala version: 2.13.10

import concurrent._, ExecutionContext.Implicits._, duration._

class C {
  def test(): Future[Int] =
    Future.unit
    .map { x =>
      throw new NoSuchMethodError("test")
      42
    }
}

object Test extends App {
  val c = new C
  Await.result(c.test(), Duration.Inf)
}

Problem

Nothing in the Scaladoc tells me that https://github.com/scala/bug/issues/12150

There ought to be at least minimal guidance. Perhaps Akka docs have a model.

The Scaladoc for Future should not say, "Don't use Future." In case that needed clarification.

som-snytt commented 1 year ago

If there is an overview page, then Scaladoc ought to link to it. The semi-structured docs are a challenge to navigate.

The new Scaladoc could also just link to ChatGPT with a useful prompt!

julienrf commented 1 year ago

The guide on Futures and Promises has a section that clarifies that: https://docs.scala-lang.org/overviews/core/futures.html#exceptions

I agree that it would be good to add a cross-reference or to repeat the same note in the API documentation.

tmccombs commented 1 year ago

That overview documentation could use some discussion of how to correctly handle Fatal exceptions. In particular, unless you provide a custom reporter to an execution context, the execution contexts provided by the standard library will simply log the fatal exception, and nothing is done with it.

There should be a recommendation to either create an execution context with a null Executor and a custom reporter (with something like ExecutionContext.fromExecutor(null, customReporter)) or use an Executor that uses an UncaughtExceptionHandler, or use custom implementation of ExecutionContext that catches fatal errors. And handles them appropriately.

I also wonder if it would be more appopriate to mark the Future as failed, and then re-throw the exception if it is fatal.

som-snytt commented 1 year ago

Scaladoc at https://github.com/scala/scala/pull/10382

Also more words in overview at https://github.com/scala/docs.scala-lang/pull/2777

I hope the example helps, as there are already a lot of words.

As we know, Future has surprising complexity. Speculatively, it would be interesting if doc pages could be linked, much like those diagrams in the intro to textbooks which show a graph of possible orderings of chapters to read, so that even if my landing page is Future, it tells me "but have you read ExecutionContext and error handling yet?"

som-snytt commented 1 year ago

from the instigating ticket

a fatal exception was getting silently swallowed.

There is a doc macro on "swallows exceptions" that is in this ball park. I think onComplete, where the idea is that you can only witness errors by inspecting reports.

https://www.scala-lang.org/api/current/scala/concurrent/Future.html#onComplete[U](f:scala.util.Try[T]=%3EU)(implicitexecutor:scala.concurrent.ExecutionContext):Unit

Also that implies or suggests that only non-fatal errors are reported and that fatal ones go somewhere else.

So part of the complexity is that everything is multivalent (or a lie). It still seems terrible that fromExecutor(executor, reporter) doesn't do the obvious thing.