Future is not completed with anything if it throws InterruptedException

[scabug]

When code executed as future throws InterruptedException, future is not completed (neither success or failure). Await.result will block indefinitely if timeout is infinite.

It is quite typical for any concurrent code to throw InterruptedException, for example Thread.sleep() might throw one. It is extremely error prone that this type of exception is swallowed and only indication is ExecutionContext's failure report, by default printed to console.

[scabug]

Imported From: https://issues.scala-lang.org/browse/SI-8938?orig=1
Reporter: Jani Räty (jraty)
Affected Versions: 2.11.2
Attachments:

• bug.scala (created on Oct 23, 2014 3:49:18 PM UTC, 986 bytes)

[scabug]

Simon Ruggier (sruggier) said:
I don't have privileges to assign this issue to myself, or I would do that. I have a fix for this that I'll create a PR for.

[scabug]

@axel22 said:
Still, this requires a mailing list discussion, as it had been a deliberate design decision. Also, if we do make this change, it's not clear what the migration cycle will look like.

[scabug]

@axel22 said (edited on Dec 30, 2014 10:39:08 AM UTC):
There is another thing bugging me with this bug.
Why would Future.apply body throw an InterruptedException in the first place? Doesn't that mean that the asynchronous code that completes the Future was blocking? If so, that code, such as Thread.sleep, should have been wrapped in the blocking call:

http://docs.scala-lang.org/sips/completed/futures-promises.html#blocking

In that case, the blocking call should be catching the InterruptedException - if it wraps the exception into something else and rethrows, there is no need to update NonFatal.

[scabug]

Viktor Klang (viktorklang) said:
It is not so simple as to just twiddle with Future.apply—what about: f.map(_ => throw InterruptedException) etc etc?

What is clear is that the InterruptedException must not propagate, as it is only intended to unwind the stack to the caller—not go across callstacks (async).

So then the question becomes: Should failures in Futures special-case InterruptedException and wrap it in something like an ExecutionException, or since Futures are not Interruptible per-se, it be the responsibility of the person who calls interruptible code to deal with interruptions?

[scabug]

Simon Ruggier (sruggier) said:
You're right, I (or the reporter) should open a thread on the mailing list. That being said, I'll respond to the comments you've made so far.

Regarding it being a design decision, yes, someone deliberately put the NonFatal filters in place, but it's unclear whether they deliberately intended for Futures to swallow fatal exceptions the way they do. I didn't dig it up before making the PR, but I found the the original commit that added the NonFatal qualifiers everywhere (af8416bb7ff4d6816f416cd0671e26e2cdc653d7):

commit af8416bb7ff4d6816f416cd0671e26e2cdc653d7
Author: Viktor Klang <viktor.klang@gmail.com>
Date:   Fri Apr 13 18:11:38 2012 +0200

    Adding NonFatal matching as to not catch anything that should terminate the JVM, also adding EX.reportFailure where it makes sense

It seems like the commit description is talking about VirtualMachineErrors, not other types of fatal exceptions. I agree with this concern, and would suggest that the Future should either rethrow them after completing the promise, or avoid catching them at all (and yes, my current PR should be updated with one of those options). For all other Throwables, including the other fatal ones, it looks like the async thread has no reason not to complete the promise with them, thus notifying the caller that there was a failure. The SIP 14 Exceptions section, and the library documentation of Future, still claim that Future will convey fatal exceptions back to the caller, wrapped in an ExecutionException. That's no longer true as of the af8416 commit.

The current documentation for blocking says that it now only is intended to cover external blocking APIs, and that Await.result should be used directly instead. As applied to third party blocking APIs, it also says that it throws InterruptedException if the blocking is interrupted. Although it's clear to me why blocking would be discouraged within Future computations, I'm not yet aware of where the documentation specifically forbids use of sleeping code from within a Future.

Regarding the extra hunks in my PR, everything that's not related to the NonFatal change is there to update the tests to verify the changes that the PR makes. I'll respond in more detail in the line notes.

[scabug]

Simon Ruggier (sruggier) said (edited on Dec 30, 2014 11:31:44 PM UTC):
Oops, I took so long to write my response that I missed Viktor's comment. Yes, it was an oversight on my part that I didn't search for other uses of NonFatal, it would make sense to update all or none of those cases, not just the one that I updated.

I agree that the InterruptedExceptions shouldn't get propagated across threads, and it looks like we're not the only ones thinking this, because that's also how the Future documentation, Promise implementation and PromiseTests.scala expect InterruptedExceptions to be handled, as they're currently written (i.e. before the change). The proposed change restores previous behaviour, and pulls into play this existing code that does wrap the exceptions in ExecutionExceptions before they're propagated to the caller in e.g. Await.result.

Edit: a few minutes later, I realized on my own that in between the catch and the rethrow, completing the promise could trigger significant computation as a result of running the completion handlers. I can understand now why that would justify the current behaviour. On the other hand, assuming that clients are sanely written (if you'll briefly excuse this laughable premise), we can expect that correctly written completion handlers would gracefully abort when they see that the last promise failed with a fatal exception. VirtualMachineErrors are catchable (at the language level) presumably to allow clients to attempt to perform necessary cleanup before they terminate, and it probably makes sense to give clients of Scala's concurrency API the same option. There's a risk that some clients may abuse this to do terrible things, but that's also true of catch blocks, and really any powerful language concept in general. I think it would be better to err on the side of flexibility than to try to make it impossible to ignore fatal errors.

[scabug]

@axel22 said:
An important remark about the docs:

The current documentation for blocking says that it now only is intended to cover external blocking APIs, and that Await.result should be used directly instead.

No, it says that you should use blocking to designate code that potentially blocks. It does not matter if the code blocks due to Thread.sleep, Await.result or an I/O operation - these are all examples of blocking.

It also says that waiting until a Future object is complete (to get its value) should be done with Await.result.

[scabug]

Simon Ruggier (sruggier) said:
I've opened a discussion on Scala internals about fatal error handling.

[scabug]

Daniel Darabos (darabos) said:

I've opened a discussion on Scala internals about fatal error handling.

Wow, that's a long discussion! It is very insightful too. I'm not sure what the conclusion is. Will this be fixed?

Let me add one more example of why the current behavior is annoying. I do some reflection in a Future and it may fail with NoClassDefFoundError if things go wrong. Since NoClassDefFoundError is not NonFatal, this does not complete the Future and the thread waiting for its completion will wait forever. This is a user request for a potentially slow operation. The user will wait hours before deciding that perhaps something is wrong.

The only workaround I see for now is that I have to become an expert at what may cause an exception that is not NonFatal and be sure to catch all those myself. It would be great if I could override the definition of NonFatal in my own ExecutionContext instead.

[scabug]

Viktor Klang (viktorklang) said:
Hi Daniel,

What's the problem wrapping the code which does the reflection in a try-catch and repackage the fatal exception for those cases where this is needed?

[scabug]

Daniel Darabos (darabos) said:

What's the problem wrapping the code which does the reflection in a try-catch and repackage the fatal exception for those cases where this is needed?

I don't want anything special to happen for NoClassDefFoundError. I'd like to have it handled just like ArithmeticException. The reflection happens deep in the call stack, no way to meaningfully handle it there. And reflection can cause so many different exceptions when things go wrong. The higher layers don't care about the reflection. To them this is the same as a division by zero. "What we wanted to do failed." So why is the Future not completed with a failure? The JVM has certainly not been destroyed.

Like I said, I could indeed try to track down all the calls where a non-NonFatal exception can happen and wrap these. But I will not find them all, and I don't want to do this in the first place. If you say Scala's Future handles a subclass of exceptions nicely, but for a few types, like InterruptedException and NoClassDefFoundError I'll have to do it myself, it's like a multiplication operator that works for all numbers except 5 and 7. "You can implement multiplication for those exceptions yourself." Yeah, I guess, but why are you excluding them?

At least that's what it feels like at the moment. But I haven't read through the whole discussion thread yet. I just wanted to ping the bug to see if there is any decision to move in some direction with this. Thanks for any insight!

[scabug]

Daniel Darabos (darabos) said:
Sorry, I don't want to swamp this ticket, but I found the example I wanted to show. The NoClassDefFoundError happens in our own code, so it's our fault. But here's another example where we lost a Future:

Exception in thread "DataManager-3" java.lang.NoSuchFieldError: HIVE_CLI_SERVICE_PROTOCOL_V8
        at org.apache.hive.jdbc.HiveConnection.<init>(HiveConnection.java:189)
        at org.apache.hive.jdbc.HiveDriver.connect(HiveDriver.java:105)
        at java.sql.DriverManager.getConnection(DriverManager.java:571)
        at java.sql.DriverManager.getConnection(DriverManager.java:233)
        at com.lynxanalytics.biggraph.graph_operations.DBTable.lines(DBImport.scala:52)

I'm just trying to connect to a database. How would I know to expect a NoSuchFieldError with DriverManager.getConnection? I'd much rather just trust Future that it will propagate any exception as a Failure.

[scabug]

Viktor Klang (viktorklang) said:
Daniel,

I think it might be worth it to read the conversation :)

[scabug]

Simon Ruggier (sruggier) said:
Viktor, I had intended to summarize that conversation a year ago, so as you suggest to Daniel, I've reread it now, and I'd summarize the outcome as follows:

InterruptedException and ThreadDeath

These should be converted to an ExecutionException and propagated to the caller. [1] [2].

It is sufficient to complete the promise with the exception to implement that behaviour [3].

ControlThrowables

These should be treated similarly to InterruptedException and ThreadDeath [4]. Completing the promise with them would also be sufficient to result in their being wrapped in an ExecutionException.

VirtualMachineErrors

There was a lot of debate about these, with Scott Carey, Rex Kerr, and I advocating to pass them on to the caller, and you advocating that they shouldn't be caught under any circumstance. The debate was ultimately unresolved, although the two of us would agree that it would be preferable to call sys.exit when VM errors leak out of a Future's execution, instead of the current behaviour.

I think another reasonable alternative would be to make another exception that is similar in meaning to ExecutionError, but that derives from VirtualMachineError, and use this to unwind the stack on calling threads. If implemented properly, this should also cause applications to exit, but in a way that may be clearer to the user. Either solution would be preferable to the current situation, where programs can simply hang if there's a VirtualMachineError.

LinkageErrors

The discussion around these was incomplete. However, none of the reasoning around VirtualMachineError applies to LinkageErrors: there's no reason to believe that the JVM is compromised after a LinkageError is thrown, so the runtime is perfectly capable of catching these and propagating them to the caller. Additionally, I've carefully reviewed the discussion, and I see that there is an undisputed consense among users that the current behaviour is very confusing and unhelpful. If the current behaviour is undesirable, and there's no outstanding reason to keep the status quo, then I'd conclude that it should change.

I think these errors should be propagated into the Future's result, so the caller can see them, and I see nothing from that conversation to suggest that this isn't a good idea. If it's considered important to avoid wrapping them in a non-fatal exception, they can be wrapped in something like an "ExecutionLinkageError" (similar to what I propose for VirtualMachineErrors), which would make it less likely that the caller catches them without explicitly intending to.