Improve and explicitly document policies regarding exception throwing

[fzhinkin]

Okio has @Throws(IOException) annotations for most of its methods and also some documentation describing which exceptions could be thrown. However, some edge cases are not always documented.

For kotlinx-io, I got rid of @Throws(IOException) and want to explicitly document possible exceptions for every method.

Below are some open questions regarding exception-throwing policies.

1. What to throw on an attempt to read/write from a closed Source/Sink?
   Okio throws IlegalStateException, while other libraries throw more IO-ish exceptions: java.io throws IOException, java.nio throws ClosedChannelException, .NET's System.IO.Stream throws ObjectDisposedException.
   I would stick to throwing IOException as users may expect IO exceptions when reading/wiring data and could handle it appropriately.

2. What to throw for negative or too large byteCount values from methods accepting only it? What to throw from methods that also accept offset?
   For the latter, the library already throws IndexOutOfBoundsException which seems to be fine.
   For other cases, it may throw IOOBE or IllegalArgumentException. It seems more appropriate to always throw IllegalArgumentException in such cases.

[swankjesse]

I think the other APIs get it wrong! Throwing an IOException everywhere conflates programmer errors (read after close) with environment errors (remote file system is unreachable). At my company we crash the app on programmer errors, whereas we attempt to retry on environmental errors.

java.io

I don’t think java.io sets a good precedent here. Here’s an example of something that grosses me out about java.io:

  @Test
  fun closeByteArray() {
    val input = ByteArrayInputStream(ByteArray(1000))
    input.close()
    assertFailsWith<Exception> {
      input.read()
    }
  }

The above test fails because ByteArrayInputStream doesn’t care if the stream is closed or not. I believe that’s pretty hostile to using ByteArrayInputStream in tests.

java.nio

For java.nio, I think ClosedChannelException conflates cancel with close. For me, cancel means ‘interrupt an I/O operation so that it fails immediately’ and close means ‘release any resources held by this stream’. Canceling is an essential feature, especially so for network I/O.

Two of its subclasses, ClosedByInterruptException and AsynchronousCloseException, signal that the IO operation was canceled by another far-away thread. But the base class ClosedChannelException is thrown if I close the channel.

In particular, I do not like how responsibility for releasing resources is structured here. If I catch a ClosedChannelException because another thread interrupted me, do I need to call close() ? I find this API inscrutable.

I think operating on a canceled stream should throw an IOException (environmental error), and operating on a closed one should throw an IllegalStateException (programmer error). Because environmental and programmer errors are conflated here, java.nio has to do IOExceptions.

.Net

I can’t speak to this as I haven’t used it.

[qwwdfsad]

operating on a closed one should throw an IllegalStateException (programmer error)

Exactly it; an attempt to use a closed source/sink is a semantical programmer's error: it is, unlike IOException, happens due to internal factors (i.e. API misuse), not external ones (e.g. the network is flaky). The former is fatal, the latter is recoverable ("catcheable"). The same approach is used in coroutines' channels, Ktor's primitives and similar standard library API (e.g. constrainOnce). I'm all for throwing ISE here as well.

For other cases, it may throw IOOBE or IllegalArgumentException. It seems more appropriate to always throw IllegalArgumentException in such cases.

We typically prefer throwing the least specific reasonable type, providing all the required contextual information in the message. I find it hard to justify why one may want a specific instance of IndexOutOfBoundsException, while there is not much to do with it.
Specifying the exception type is always a backwards-compatible change, while the opposite is not true, so IAE seems to be a good fit here.

Regarding java.nio.* -- I'm not really sure it is a good API model to follow, it has quite a few issues with separations of concerns and has, overall, quite a complex surface

[fzhinkin]

Regarding IOOBE and IAE - I'll follow the same policy adopted by the Kotlin stdlib for index-based methods:

IndexOutOfBoundsException - when startIndex or endIndex is out of range of source array indices.
IllegalArgumentException - when startIndex > endIndex.

And throw IAE for all other methods in case argumets don't pass the validation.

'ISEfor closed sources/sinks andIOException` for canceled also sound good.