Should runInterruptible throw InterruptedIOException as CancellationException?

[ursusursus]

When used with retrofit/okhttp/okio stack, reading and writing bytes becomes

val source = api.download("..").source()
source.use { it.readAll(sink) }

However the read/writes become blocking and non cancellable. Fortunatelly okio checks for java thread interrupts, so runInterruptible can be used

val source = api.download("..").source()
runInterruptible { 
   source.use { it.readAll(sink) } 
}

However, okio throws java.io.InterruptedIOException, which the runInterruptible doesn't then repackage as CancellationException

Should be repackage this exception as well?

private fun <T> runInterruptibleInExpectedContext(coroutineContext: CoroutineContext, block: () -> T): T {
    try {
        val threadState = ThreadState(coroutineContext.job)
        threadState.setup()
        try {
            return block()
        } finally {
            threadState.clearInterrupt()
        }
    } catch (e: InterruptedException) { <---------------------- 
        throw CancellationException("Blocking call was interrupted due to parent cancellation").initCause(e)
    }
}

[dkhalanskyjb]

I'm not sure. square/okhttp#3945 says that OkHttp will throw InterruptedIOException even in the case when it's not a normal thread interruption but instead a timeout, the difference is in the interrupt flag, which runInterruptible unconditionally clears. We certainly don't want timeouts to be treated as cancellation. Though that discussion is quite old, maybe something has changed since, but I didn't manage to quickly find anything more up-to-date.

[ursusursus]

@JakeWharton care to comment?

[JakeWharton]

@yschimke maintains OkHttp

[yschimke]

I'll take a look, and happy to fix specific issues which are obviously not correct.

General context...

I'm wary about promising too much for OkHttp here. Our draft north star for cancellation is this square/okhttp#7185, but its still advising against interrupts because they are likely bad for performance of pooled connections.

In truth there are a bunch of edge cases that happen at the moment during new stream creation, that can manifest as bugs under interrupts. So double wary here. Post response is probably fine.

[yschimke]

Making this change above, and swallowing timeouts accidentally via runInterruptible does look problematic. Since both Okio (via non-clearing Thread.currentThread().isInterrupted), and okhttp (via Thread.currentThread().interrupt()) retain the interrupt status, perhaps a simple fix is to swallow InterruptedIOException when the thread interrupt flag is also set?

Aternatively, looking into this for okhttp change, I suspect it's not just okhttp, but also okio that would need to change here.

okio - timeout and cancellation as InterruptedIOException
https://github.com/square/okio/blob/5086d945ab5eebbe63d45a15ea9cc69197140d0a/okio/src/jvmMain/kotlin/okio/Timeout.kt#L95

okhttp - test confirming timeout as InterruptedIOException
https://github.com/square/okhttp/blob/58ee1ce170c2aea75a113012956260873e808dad/okhttp/src/jvmTest/java/okhttp3/internal/http2/Http2ConnectionTest.kt#L1470

So @swankjesse for Okio and OkHttp...

I suspect a part of reason OkHttp deliberately throws InterruptedIOException, is that internally the code cleanly handles IOExceptions but not runtime exceptions. We also don't want to do any other IO after this via retries, so retaining the interrupt status up to caller code is a safe default.

Technically I think Okio is breaking the contract of InterruptedIOException, but arguable. Because of this my suggestion in the first line of checking the status, appears okio/okhttp specific, it may not apply to any other libraries.

Signals that an I/O operation has been interrupted. An InterruptedIOException is thrown to indicate that an input or output transfer has been terminated because the thread performing it was interrupted.

[swankjesse]

For APIs written to support non-coroutines users, CancellationException is a non-starter.

I recommend changing runInterruptible to recover gracefully from InterruptedIOException.

Note: I previously responded as if this was an issue reported against OkHttp. My bad!

[dkhalanskyjb]

perhaps a simple fix is to swallow InterruptedIOException when the thread interrupt flag is also set?

We would need to look at how InterruptedIOException is used in other places to make this judgement, but in general, this fix does have issues: it's racy. The following is possible:

1. Actual cancellation happens and sets the interrupt flag.
2. Independently, in parallel, InterruptedIOException is thrown to indicate an error. The interrupt flag should not be set.
3. We check the flag, see the InterruptedIOException and swallow it.

In this case, where "1" takes effect immediately and the error indicates a timeout, this could be fine: we all could just pretend that 2 didn't happen and 1 directly led to 3. It's a close call anyway.

[dkhalanskyjb]

So, I've looked into it. Seems like people really do use InterruptedIOException interchangeably with InterruptedException. However, as to what this exception actually idiomatically means, I don't think there's an answer.

The docs state:

An InterruptedIOException is thrown to indicate that an input or output transfer has been terminated because the thread performing it was interrupted.

However, https://docs.oracle.com/javase/7/docs/api/java/net/SocketTimeoutException.html does not seem to behave this way:

    val url = URL("https://www.google.com")
    Thread.currentThread().interrupt() // <-------------------------- no effect
    val conn: HttpURLConnection = url.openConnection() as HttpURLConnection
    conn.connectTimeout = 1000 // remove the zeros to observe the exception
    try {
        println((conn.content as InputStream).readAllBytes().toString(Charsets.UTF_8))
    } catch (e: InterruptedIOException) {
        println(Thread.currentThread().isInterrupted)
    }

I see some HTML being printed, so this just… ignores that the thread was interrupted. Maybe I'm missing something?

So, we have three interpretations of InterruptedIOException:

1. Per the docs and found in the wild: exactly like an InterruptedException, but with additional information, clearing the flag before throwing (a typical pattern is like https://github.com/Goooler/MaterialFiles/blob/5cfa925af1c6dcb337f921973a1f3e3c873f6d8c/app/src/main/java/me/zhanghai/android/files/filejob/FileJobs.kt#L218-L220). This is by convention: "any method that exits by throwing an InterruptedException clears interrupt status when it does so" (from https://docs.oracle.com/javase/tutorial/essential/concurrency/interrupt.html)
2. Per how it behaves in JDK's own code: just some exception that denotes a timeout. Will preserve the interrupt status and, in fact, will completely ignore it.
3. OkHttp's version: both the timeouts and an InterruptedException, but with the flag's meaning reversed: a timeout will not have the flag set, but an interruption by a thread will.

I'd say, with all this, we should never swallow an InterruptedIOException in runInterruptible, not when the flag is set (or we would lose the information about a timeout in HttpUrlConnection), and not when the flag is unset (or we would lose the information about a timeout in okhttp/okio).

[swankjesse]

We preserve the interrupted state in OkHttp so that catching an IOException doesn’t lose information.

try {
  return callPrimaryServer()
} catch (e: IOException) {
}
return callSecondaryServer()

In this sample, if the thread is interrupted we shouldn’t just fall back to the secondary server.

[dkhalanskyjb]

Good to know, thanks, that's a solid reason! Though I don't see any downsides to just throwing InterruptedException when it's the thread interruption and InterruptedIOException when it's the timeout. Though this ship has probably sailed.

In any case, this is irrelevant to the discussion, which is about whether or not we, the coroutines library, should catch InterruptedIOException when we catch InterruptedException. The answer seems to be that, given the state of things, we should not, as this would cause issues with either the code using OkHttp or the code using HttpURLConnection (and who knows what else).

[ursusursus]

@dkhalanskyjb so catching InterruptedIoException and throwing InterruptedException so runInterruptible kicks in, is bad?

[dkhalanskyjb]

If you know you're using it with OkHttp, by all means, do this, just don't forget to check the interrupt flag. We are a general-purpose library and can't write code specifically tailored to OkHttp at the expense of other use cases, but in your own code, you can do whatever you want!

[yschimke]

@dkhalanskyjb if we can definitively state that some of these are wrong, we can definitely look at changing this in OkHttp 5. As square/okhttp#7185 shows, we'd like to be good interrupt citizens.

[dkhalanskyjb]

I don't think we can definitely state anything conclusive about InterruptedIOException, as its use in the JDK contradicts the docs. All I can say is that if you throw InterruptedException on a thread interrupt, then any code that processes interruptions, like our runInterruptible, will automatically work, and the problem stated in #3551 (comment) will still be avoided. However, maybe you do want the exception that signifies a thread interrupt to still be an IOException for some reason. In that case, I don't see any sensible solutions other than the one you already have.