boundary/break leaks implementation details and may cause unexpected behaviour

[szymon-rd]

Compiler version

3.3.0

Minimized code

A simple example of a code that contains behavior unexpected by the user, caused by implementation detail of boundary/break:

//> using scala 3.3
import java.net.http.HttpTimeoutException
import scala.util.boundary
import boundary.break

object Main extends App:
  boundary:
    while true do
      try
        makeRequest()
        break()
      catch
        case _: Exception => //just retry
  println("Finish")

  var c = 0
  def makeRequest() =
    println("Making request")
    if c < 3 then
      c = c + 1
      throw new HttpTimeoutException("timeout")

This loop would never finish, as it would first catch the real timeout exception and then all the Break exceptions. It makes perfect sense it works that way, given how the boundary/break is implemented. However, catching all the Exceptions on retries when, e.g., waiting for a service to start responding, is a widespread pattern. Scala users writing simple programs like this must be aware of this implementation detail.

What's more - that's a basic example. Along the way, library maintainers will use the boundary/break in their libraries. Given an additional layer over this mechanism, the leaking implementation detail will be even more misleading.

To solve the antipattern of catching Exception, we offer the NonFatal with unapply. However, Break will be caught as NonFatal by the users, so it won't help as well. It may even cause more confusion, as the previous implementation detail of breaking (the ControlThrowable) was not matched by NonFatal and was sometimes used as a solution.

Proposal

After talking about it, we propose that the compiler could report an error if there is a Label in the implicit scope and the Break is caught indirectly (i.e. by matching Exception). It would prevent the users from experiencing the leakage of implementation detail and the possible confusion when their code does not work as expected. Nevertheless, I would like to hear some feedback on that.

CC @odersky @bishabosha @sjrd

Update: Problem with the solution we proposed is that it would not solve the problem of runtime matching in unapplies (so, for example, NonFatal)

[sjrd]

I argued for a long time that BreakException should extend ControlThrowable, but @odersky was adamant that it must be a regular Exception, in order to make it clear what it does. 🤷‍♂️

After talking about it, we propose that the compiler could report an error if there is a Label in the implicit scope and the Break is caught indirectly (i.e. by matching Exception). It would prevent the users from experiencing the leakage of implementation detail and the possible confusion when their code does not work as expected.

It won't achieve that. With one more indirection, the problem becomes invisible to what you are proposing:

  boundary:
    loopForeverEvenOnExceptions:
      makeRequest()
      break()
  println("Finish")

  def loopForeverEvenOnExceptions(body: => Unit): Unit =
    while true do
      try
        body
      catch
        case _: Exception => // loop forever anyway

[szymon-rd]

Yeah, you are right... Another hacky solution: exclude Break type from every pattern matching in catches/on exceptions, as long as it's not specified directly (i.e. either by just : Break, or union type including it directly).

[sjrd]

We cannot do that. That would undeniably make it a language feature, while it is currently nothing but two library functions.

Also, that still breaks if the try is in Java or JavaScript.

[odersky]

No, I think we have to change the mindset of people instead. Breaks are aborts and there should be only one kind of abort. And everytime I see a "just ignore any exception thrown and retry", alarm bells start ringing in my head. That's just a recipe for infinite loops. and we should call it out.

Here's another, better example:

//> using scala 3.3
import java.net.http.HttpTimeoutException
import scala.util.boundary
import boundary.break

object Main extends App:
  boundary:
    openResource()
    while true do
      try
        makeRequest()
        break()
      catch
        case ex: Exception =>
           log(ex)
           closeResource()
           throw ex

Since breaks are aborts, the resource will be closed when the break happens, which is what we want.

[szymon-rd]

Catching all exceptions, and ignoring them, is definitely an antipattern. However, many Scala programmers often write a short script and want to do it as quickly and simply as possible. I've seen people report this problem already. One of them is my friend, who is experienced in Scala, and generally a programmer who is very aware of antipatterns and writes solid code. In this case, however, he wrote a concise script waiting for HTTP service and couldn't figure out what was happening because he didn't expect the control to rely on throwing Exception .

And that's what our users will do, so even if we want to discourage this mindset and enforce a different one, we should do it as painlessly as possible - via errors or warnings. Otherwise, it could be a frustrating DX problem. Maybe we should start with some errors discouraging this approach. I see two possible directions right now:

1. Report warnings on all case ex: Exception with an empty/very simple body (for example, ones that do not reference ex). If that's an antipattern that can lead to unexpected behavior, we should be strict about it.
2. Reports warnings on all case ex: Exception, when there is Label in scope. This is problematic because it will only cover some cases, as @sjrd shows in his example.

It only solves some things, but it could be a start. I think covering the most common cases can already help massively.

[odersky]

I think we should try make it a linter warning if

1. We catch something that is a possible supertype of BreakException
2. We have a boundary.Label in scope
3. We don't obviously rethrow the exception. This could mean:
   • no throw in the handler at all, or
   • no throw in one of the paths of the handler
     I think it's OK if we disregard throws in called methods.

[szymon-rd]

I was playing with boundary/break to summarize all cases when it could happen. This one I found particularly interesting, as it doesn't involve any obvious antipatterns (because of NonFatal usage within Try). It's a real-life example of what @sjrd showed.

import scala.util.boundary
import boundary.break
import scala.util.Try

@main
def main() =
  boundary:
    while true do
      Try: 
        println("Foo")
        break()
  println("Finish")

Could we add Break to the pattern match in NonFatal? It would only solve this problem in stdlib and some custom libraries. However, we encouraged the usage of NonFatal to avoid catching control exceptions so that we would keep following this standard that way. Another solution is to discourage passing lambdas requiring Labels to hof altogether (a bit extreme, though; we can go around that with simpler heuristics). It's also easy to argue that it is a bug, so changing the NonFatal should not be breaking compat.

[som-snytt]

I don't understand the use of the word "abort" or why it isn't a ControlThrowable (as it seems to fit the very definition), so please excuse my ignorant lack of context, but I wondered if it was considered to make it an Error, which communicates "don't catch me ever".

I'm not sure about the "resource" example, because I would expect finally for that use case. IIUC, the example demonstrates intentionally catching, but I've got it in my head that not catching is better.

Perhaps the sophisticated linting under discussion will make it all clear to me.

I noticed Scala 3 does not warn for catch-all:

scala> def f(i: Int): Int = try 42 catch (e => 27)
                                            ^
       warning: This catches all Throwables. If this is really intended, use `case _ : Throwable` to clear this warning.
def f(i: Int): Int

IIRC Seth insisted on a warning when I backported this syntax. (I like the simplicity of catch f for quick code, so I don't mind the nagging message. Similarly, I agree with the comment that quickie code is an important use case and even expert users benefit from extra messaging. If I'm coding something quick, even one-off, I especially don't want to spend any time debugging a dumb mistake. On the forum, they even proposed linting false | true. My keyboard is getting old, maybe the VBAR key doesn't repeat like it used to.)

[szymon-rd]

@som-snytt its already released, the problem is how to fix this issue while keeping our compatibility guarantees. So no changes in types.

[dwijnand]

I don't understand the use of the word "abort" or why it isn't a ControlThrowable (as it seems to fit the very definition), so please excuse my ignorant lack of context, but I wondered if it was considered to make it an Error, which communicates "don't catch me ever".

Context:

• Introduce boundary/break control abstraction. #16612 (comment)
• https://contributors.scala-lang.org/t/nonfatal-and-controlthrowable-changed-in-stdlib/6049

[odersky]

We went over this at length before. Yes, we certainly do want Try to catch a break abort. If we don't do that we risk deadlocks in code that uses future.

It's really an education problem. Unfortunately the Scala community was blinded for too long by the weak and leaking abstraction of NonFatal. The gist is: Exceptions and break are both aborts, and all aborts behave the same. Once you accept that, everything makes more sense.

[szymon-rd]

If we want to move with boundary/break and NonFatal with a shared vision, and take care of their usability, we need to paint the whole picture and answer some questions. So, to summarize it:

Problem addressed by current boundary/break

ControlThrowable was excluded from NonFatal. It caused issues with execution engines, as it's not a fatal Error. It's just an abort that Future and execution engines cannot handle, and we cannot express that in the language in its current form. It all led to ControlThrowable not being treated as a regular Exception, but as a fatal one and reported as such, leading to more problems (possibly including deadlocks).

Additionally, there were arguments made against the sensibility of NonFatal in its current form, such as:

• Error is already there and is meant to represent fatal (in our definition) errors,
• NonFatal lost its clear meaning and intent. It excluded InterruptedException, which is not really fatal, but it is useful to think of it as such from the execution engines' perspective. On the other hand, excluding ControlThrowable made no sense from the execution engines' perspective.

There are more benefits, but they are out of the scope of this discussion.

Solution provided by boundary/break

Break was introduced as a standard RuntimeException. Now it's being caught by all the handlers in execution engines and treated as a standard Exception.

Rationale for the current shape of boundary/break

Primarily, it solves the problem it was meant to solve.

Additionally, a conceptual framework exists where Exceptions and breaks are both aborts and should be treated the same. Given that our users think that way, it may provide a consistent way to think and operate on them.

Break was not made a Throwable to allow replacement of NonFatal extractor with catching an Exception. After that change, we restored the division of Throwable into Error and Exception.

Problems resulting from current boundary/break

Our users find it counterintuitive that break "control instruction" can unexpectedly change behavior when used together with error-handling mechanisms. A consistent conceptual framework exists where Exceptions and breaks share their behavior, and it makes sense that one may influence another. In this framework, both are aborts, it's not just "error handling" and "control instruction" anymore. However, it is not a way of thinking employed by the majority of programmers, even including Scala programmers.

Speaking about the usability of these features requires us to consider how our users will think about it. Changing the way of thinking may provide additional value. Still, at its core, it is a cost. It negatively affects usability - causing initial confusion and possible errors that may be even worse than the ones caused by the previous way of thinking. And that happens with boundary/break - we have a breaking mechanism conceptually incompatible with solutions used in other popular languages, and it causes confusion and frustration. Worse, it's incompatible in a hidden way that will appear only in runtime. And it won't cause any errors. It will just work differently. Our user can even not notice that until, e.g. in production.

Proposed solution

The first proposal was to introduce warnings. That may help as a complementary or temporary solution, but more is needed to solve the core problem. Warnings are easy to miss and won't help with the confusion and frustration - users will still perceive this behavior as odd.

After some internal discussions, we arrived at the idea of the following components.

Introduce NonControl extractor

NonControl would match all Exceptions that are not Break / other control exceptions.

By making Break an exception, we shifted the responsibility from execution engines to users to avoid catching Breaks. What was previously expressable with catching an Exception now lacks a simple way to express it. The only way to catch an exception, as most programmers understand it, is with additional pattern guard checking if it's an instance of Break.

NonControl will have an advantage over the NonFatal - it will have a clearly defined purpose, it won't introduce confusion leading to errors in execution contexts, and its semantics will match its name. Therefore, we should introduce NonControl.

The final name of this extractor is still to be discussed.

Fix semantics of Try

Try and Future share similar apply methods. However, they are different and have different capabilities. In its current form, it's possible to implement boundary/break in Try and avoid confusing behavior. It's not true for Future, and, therefore, Future should not have the same exception handling logic as Try. Therefore, Try should use NonControl extractor instead of NonFatal. We should also exclude InterruptedException to avoid breaking existing code that might have relied on that.

One counterargument is that Future can return a Try due to the presence of, e.g., the value function. As a result, we could get a Failure[Break] that would not be possible to achieve via execution of Try.apply function. However, it does not introduce any contradiction. The execution model within the Try apply and the Try as data type are already capable of this behavior. One may want to express a failed Future with Failure[Break], if that's what happened. However, it's not a Failure in the context of Try.apply execution.

Add linter rule for catching Exception

After introducing the NonControl extractor, we can report every catch of case <...>: Exception => and suggest replacing them with NonControl. If somebody wants to catch a Break exception in such a block, writing that explicitly will be a good practice to make the warning disappear.

Deprecate NonFatal

After these steps, NonFatal will be replaceable with NonControl in some clearly defined cases. In others, just catching Exceptions will be sufficient.