Cats-effect: Properly deal with InterruptedException on the JVM

[alexandru]

We have our own NonFatal implementation because the standard one also filters things like ControlThrowable, which is pretty dumb.

However this means we are no longer catching InterruptedException, which is a problem.

The problem is that on top of the JVM, when you catch an InterruptedException, you can't just swallow it like any other exception. This is a problem:

try {
  operation()
} catch {
  case ignored: InterruptedException => /* BIG PROBLEM! */
}

What needs to happen is that we need to "restore the interrupted status". This can be done like this:

try {
  operation()
} catch {
  case ignored: InterruptedException =>
    // Restores interrupted status
    Thread.currentThread().interrupt()
}

So we need a NonFatal specific to the JVM that, upon catching InterruptedException, it restores its interrupted status.

[mpilquist]

@alexandru It's not clear to me that NonFatal should reset thread interruption status. I can see an argument for making that the responsibility of the user. Have you given any thought to the tradeoffs of auto-propagating interruption status vs current behavior? Seems like this is a very use case specific issue.

[akilegaspi]

I agree with @mpilquist , I think it's a matter of how what kind of operation the user actually spawned in the thread hence the responsibility of handling the thread interruption falls on the user's responsibility since not all operation being spawned in that context may be interrupted.

Also maybe the user might want to do something if they interrupt the thread on purpose.

[alexandru]

@mpilquist @akilegaspi it's not the responsibility of the user.

This issue is about the interoperability of Java libraries — the interruption protocol for Java threads is to:

1. let the thread die by re-throwing the exception, or ...
2. resetting the interrupted signal, such that the next call to interrupt() can re-throw the InterruptedException again

Consider this ExecutionContext:

class SingleThreadContext extends ExecutionContext {
  private val queue = new LinkedBlockingQueue[Runnable]()
  val thread: Thread = 
    new Thread(new Runnable {
      def run(): Unit =
        while (true) {
          val r = queue.poll()
          try r.run() 
          catch { case NonFatal(e) => e.printStackTrace() }
        }
    })

  locally(thread.start())

  def execute(runnable: Runnable): Unit =
    queue.offer(runnable)
  def reportFailure(cause: Throwable): Unit =
    cause.printStackTrace()
  def shutdown(): Unit =
    thread.interrupt()
}

That's the provided environment. And then:

implicit val ec = new SingleThreadContext()

// The user's responsibility ...

val io = IO.shift *> IO {
  // some slow processing
  while (true) {}
}

io.unsafeRunSync

// NOT the user's responsibility ...

ec.shutdown()

Why I'm saying that this isn't the user's responsibility is because the environment is often provided for you.

Now I don't care about Java's interruption mechanism, I don't want us to cancel the computation in case an InterruptedException occurs (although this is always a possibility). But we need to play nice with other Java libraries and reset the interrupted flag, because apparently other Java libraries expect it.

And the change is simple really:

object NonFatal {

  def apply(t: Throwable): Boolean = t match {
    case _: VirtualMachineError => false
    case _: InterruptedException =>
      // resets interrupted status
      Thread.currentThread().interrupt()
      true
    case _ => 
      true
  }

  def unapply(t: Throwable): Option[Throwable] =
    if (apply(t)) Some(t) else None
}

What will happen is the behavior we have now, except that Thread.currentThread.isInterrupted() currently returns true and after this change it would return false. That's the protocol apparently — if you catch it and you won't re-throw it, then this flag needs to become false.

[mpilquist]

Ah, I thought you wanted NonFatal to return true on InterruptedException, and hence I felt the user should decide what to do with the exception -- e.g., rethrowing, reinterrupting, or ignoring. Based on your change above, I see that you are suggesting the opposite -- that NonFatal does not extract InterruptedException. In that case I'm totally on board with the suggested change.

[alexandru]

@mpilquist actually that was a typo 😀

The current custom NonFatal catches InterruptedException and without some changes I now think that's a mistake.

So we have three choices:

1. we don't catch it, like Scala's standard NonFatal
2. we catch it and stream it as an error, like we currently do, but we reset its interrupted status
3. we catch it and treat it as cancelation

So what would be your vote?

[mpilquist]

My vote is 1 -- don't catch it at all and be consistent with scala.util.control.NonFatal.

How IO behaves when an InterruptedException is thrown is orthogonal though, right? That is, we can still treat an InterruptedException as cancelation in IO by adding an additional match?

[alexandru]

If we don't catch it in NonFatal, then we won't execute cancelation handlers and bracket for example won't work.

Should we treat it as a cancelation signal, by adding an additional match? I don't see anything wrong with it, since thread interruption is Java's notion of cancelation.

[mpilquist]

Okay I see. I guess 3 then. :)

[kubukoz]

If https://github.com/monix/monix/pull/655 gets merged, I'll follow up with a PR here as well ;)

[durban]

Should this really be closed? As far as I can tell, the "deal with InterruptedException" part is not done yet ...

[alexandru]

The switch to Scala’s NonFatal is enough for now. Not catching InterruptedException is a good solution.

In the future we might do more, like treat it as cancellation, however:

1. Any changes to the run-loop have to be accompanied with benchmarks proving we don’t get a performance regression
2. If we treat it as cancellation, it needs to be consistent, so we have to protect everything and I’m not entirely sure that we can, given IO.Async tasks, because every line of code can throw, including lines of code not under our control

Plus somebody would have to work on it, personally I’m not interested and we’ve got other priorities atm. In the future we might get back to it, but I’d prefer clean issue trackers. We’ve got enough noise as it is.

[durban]

every line of code can throw, including lines of code not under our control

I don't think that's true. Lines not under our control, yes they can throw anything. But code like 1 + 1 won't magically throw InterruptedException, only the interrupted thread state is set magically.

But anyway, I don't really care if this specific issue is closed or not; I only commented because I thought it might've been closed unintentionally.