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.

[odersky]

I think it's essential that both Try and Future also catch Break exceptions.

Otherwise the following code would lead to a race condition when determining the result of boundary.

   boundary:
      val f = Future { break(1) }
      f.onComplete: r =>
        println(r.get)
      2

[sjrd]

Future {} would catch the exception. Try {} wouldn't. That doesn't prevent Future.onComplete to pass a caught Break in a Failed to its handler. A Failed instance can and has always been able to store an arbitrary Throwable.

That said, if Break is caught by Future {}, that snippet is going to have a race condition. Ironically it will be deterministic if it's not caught. In that case the only possible outcome is to return 2. So I'm not sure what's the point being made here.

[bishabosha]

Ironically it will be deterministic if it's not caught. In that case the only possible outcome is to return 2.

But if future lets the break escape, then it will be non-deterministically caught by the boundary? (or do you mean the .get is now going to throw non-deterministically)

[sjrd]

If Future {} does not catch the Break, it will be sent for external reporting to the ExecutionContext, and the future itself will never complete.

If it does catch the Break, then it will be passed as a Failed to the onComplete handler.

[odersky]

Yes, if two futures both break they race for the value returned from boundary. The principal idea of a future is that all possible outcomes are accessed when the future's result is demanded. We want to keep that model.

[odersky]

If Future {} does not catch the Break, it will be sent for external reporting to the ExecutionContext, and the future itself will never complete.

And this would be fundamentally broken, in my opinion. Why do that but treat exceptions differently?

[sjrd]

I don't think anyone suggests that Future {} should not catch Break. It does not seem like there is a reasonable use case for that.

However, it seems most (all) reasonable use cases want Try {} not to catch Break.

[odersky]

But a Future returns a Try. So sometimes the Try catches a Break and sometimes not? 😕

[sjrd]

Sometimes a Try (Failed) instance contains a Break, sometimes not. That's fine since it contains a Throwable.

Try { ... } never catches Break.

[odersky]

Try { ... } never catches Break.

In what sense? I thought Try as it is now does catch Break, and I am arguing that's the correct thing.

[sjrd]

Yes, and everyone else in this thread says it's the wrong thing. 🤷‍♂️ That's why this thread is still going on.

[odersky]

I agree that NonFatal should be deprecated, but think we should recommend that people use Exception instead. As per JVM spec Errors should not be caught.

An Error is a subclass of Throwable that indicates serious problems that a reasonable application should not try to catch.

An operating system-like program such as Akka might want a finer distinction. That's OK, but don't publish that as the best practice to users.

[sjrd]

I think that the point people want to make is that Break should have been an Error all along, according to that criteria, because the majority of use cases don't want to catch it.

Future { ... }, like Akka, might be considered "system-like" and be allowed to catch Break even though it would be an Error in that scenario.

Note that exactly this issue came up when we designed boundary/break in the first place. You were adamant that Break should be caught as a default, but now everyone in this thread suggests that it should not be.

[odersky]

I think that the point people want to make is that Break should have been an Error all along, according to that criteria, because the majority of use cases don't want to catch it.

Emphatically disagree. If you want to catch & handle all aborts, Break must be included. An Error is something fatal that should not be handled and that will terminate the program. Break is decidedly not that. It should never terminate a program.

[SethTisue]

fyi @patriknw

[sjrd]

Emphatically disagree. If you want to catch & handle all aborts, Break must be included. An Error is something fatal that should not be handled and that will terminate the program. Break is decidedly not that. It should never terminate a program.

Given that, after everything that's been said in this thread, you are still convinced that Break should be caught by default, I think we've reached a dead end for this whole issue.

[szymon-rd]

In my summary, I was not proposing that Break should be an Error. There are arguments for and against that, and there is not much we can do now anyway. So I will not argue for that.

What I was proposing was making the behavior more intuitive. One way to do that is to acknowledge that Future and Try have different capacities regarding interactions with breaking mechanisms and to make it expressable - given that it is, per our users' reports, the behavior they expect.

I thought Try as it is now does catch Break, and I am arguing that's the correct thing.

It does, and part of the solution of the usability problem is changing this behavior - making the Try support breaking and avoiding the confusion that would arise from a lack of its support. One of the arguments for that, a simple one, is that it would be a bad practice to use Try as a way to control the Breaking mechanism - the boundary does that, Try won't bring anything aside from confusion.

But a Future returns a Try. So sometimes the Try catches a Break and sometimes not? 😕

Try would always behave the same (in this case - not catch the Break). The Future would catch and express the Break with the Try data type. Even now, it's possible to create a Try with NonFatal, despite Try not catching them. This argument requires acknowledging that there is a distinction between capacities of Try apply method, and the Try as data type when expressing a result of a calculation (that may come from a different place than the Try's apply method).

@sjrd I think that even without changing Break to Error, we can address this issue with the steps I suggested in my comment:

• NonControl extractor
• Fixing Try semantics
• Linter rule
• Deprecated NonFatal

[adamw]

A bit late in the game, but I think one aspect that wasn't mentioned is that exceptions are often caught because they are exceptional, while breaks might be quite expected and part of a "normal" program flow.

In the example:

//> using scala 3.3
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)
           throw ex
      finally  closeResource()

(apart from moving closeResource to finally) I think logging break exceptions would be most uninteresting, and probably a bug, not a feature. Very often we want to log any exceptions that occur, but given that they represent exceptional situations - that is, something's wrong. The break() here seems completely ordinary and something I definitely wouldn't want to see polluting my logs.

[odersky]

A bit late in the game, but I think one aspect that wasn't mentioned is that exceptions are often caught because they are exceptional, while breaks might be quite expected and part of a "normal" program flow.

I am aware of that. But if we want to make progress here we have to push back against this expectation. Breaks and exceptions are both aborts, and we certainly don't want to have two kinds of handling aborts with weird feature interactions between them. What is exceptional or not is very much in the eye of the beholder.

[adamw]

Breaks and exceptions are both aborts

Ok, maybe I misunderstood the feature. I was under the impression that boundary/break is meant as a control mechanism, and the fact that exceptions are under the hood is an implementation detail. But it seems it's meant for exceptional situations and signalling errors. Which is fine of course, just needs education and documentation :)

[odersky]

Agreed. In my world-view, exceptions are also a control mechanism, and programs need to take account of both breaks and exceptions. For instance when it comes to resource safety.

I believe, traditionally, this conception that "exceptions are for exceptional cases therefore I can ignore them for now" has done a lot of harm, and has caused some parts of the community to argue against exceptions altogether. But exceptions and breaks are really the same, and once we use higher-level abstractions like ? that's built on break, you can see that as a saner exception mechanism where static typing guarantees that every abort is handled. Consequently, it then makes no sense anymore to decree "sane exceptions like ? behave like this, but old exceptions behave like that". They should of course behave the same.

[tmccombs]

Another point I don't see mentioned is performance.

Even if you ignore the annoyance of having to explicitly handle Break if you use Try or catching Exception, it prevents, or at least makes it more difficult, to optimize break as a goto instead of a throw.

And even if you consider a break to be exceptional/an abort, I think that wanting to catch any non-fatal exception that isn't Break or similar is probably common enough that is worth having a NonControl unapply and an alternative constructor for Try that doesn't catch Break.

[tmccombs]

I wonder if it would be worth adding methods to the Try companion object that provide different types of error capturing something like (bikeshed on names):

object Try {
  def catchExceptions[T](r: => T): Try[T] = try Sucess(r) catch { case e: Exception => Failure(e) }
  def catchNonError[T](r: => T): Try[T] = try Success(r) catch {
    case t: Throwable if !t.isInstanceOf[java.lang.Error] => Failure(e)
  }
  // An optimizer could potentially use this to determine that a break can be rewritten as a jump instead
  // of a throw. 
  def catchExceptionBesidesBreak[T](r: => T): Try[T] = try Sucess(r) catch {
    case e: Exception if !e.isInstanceOf[Break] => Failure(e)
  }
  // this one may be ill-advised
  def catchAll[T](r: => T): Try[T] = try Success(r) catch { case t: Throwable => Failure(t) } 

[odersky]

break is implemented as a goto when that's possible.

I think we need to discuss new additions to Try in another issue targetted at the Scala library.

I also think everything that needed to be said on this issue is said, so I am closing it.

[tmccombs]

break is implemented as a goto when that's possible.

I'm aware. My point is that currently if you use break() inside of a Try, that isn't possible. But if the example in #17737 (comment) is modified to be

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

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

then hopefully the break could be implemented as goto in that case as well.

[odersky]

I see. Thanks for explaining. Yes, you could do that. But generating an exception is not that expensive -- about the cost of 3 virtual method calls according to John Rose. So I am not sure it matters much.

[Ichoran]

@tmccombs - I think the solution is just to use a different library.

I rewrote everything in my library to not use Try, not use NonFatal, and instead permit control flow (but not ControlThrowable since it's deprecated, basically).

The nice thing about that is that when you see standard constructs you know, "Oh, this clobbers control flow", and when you see the other ones, you know, "Oh, this clobbers your threads if you mess up control flow". So you at least know what to be careful about.

I don't see any particular reason the standard library needs to support both. You can't really use Try because the bulletproof principle--that Try has safe methods, not just a safe apply--has to work differently in both cases, and you really want the types to reflect "I am bulletproof in that I eat control flow too" vs "I am bulletproof but I let control flow pass as not-a-bullet".

[tmccombs]

I rewrote everything in my library to not use Try, not use NonFatal, and instead permit control flow

That isn't always an option. Specifically, if you need to interact with other scala libraries that do use Try, NonFatal, etc. And these types are also used in other parts of the standard library as well, including in Future, Using, etc.

[Ichoran]

these types are also used in other parts of the standard library as well, including in Future, Using, etc.

Yes, so I rewrote those, too.

In the case of libraries where you necessarily have lots of code in, outside of, around, etc., their uses of these things, well, yes, just use those. But they probably weren't thinking that control flow would escape, so enforcing that it can't is fine.

It's still nice to have the control flow constructs available for those patches of code where you don't have to interface with some library that's deeply dependent on control-flow-eating standard library constructs. For instance, exiting a Try block with an early success or failure is a really common use case; the failures you can just throw, but the successes are a pain to manage. Even if you're interfacing with a library, having a solution for that is nice.

Try:
  for x <- xs yield
    if just(x) then // Help, how do I stop with just g(x)?
    else f(x)

But not having to worry about thread escape is also nice.

Try:
  for x <- xs yield
    val y = Future:
      if just(x) then // Ack, what if I attempt to jump out to the Try?!
      else g(x)
    h(y)

So, having two sets of constructs with different safety guarantees is handy enough. In my library, the first one is easy and the second is a danger. (If you use type inference, most of the dangers are caught automatically, but still.)

Err.Or:
  for x <- xs yield
    if just(x) then Is.break(g(x))  // Early exit with success value!
    else f(x)

Err.Or:
  for x <- xs yield
    val y = Fu:
      if just(x) then Is.break(g(x))   // Argh, tried to break to top!!!
      else g(x)
    h(y)

One could argue which should be the primary one, but having both is strictly better so it's not such a big deal which way the standard library goes as long as there's an alternate that isn't too hard to create or get. (My library is not meant to be stable and relied upon, but Ox is.)