Exceptions and Typed errors

[nvkleban]

Hi! Thank you for the library, it's really helpful.

While investigating it I found that it's requires wrapping packageInstaller.createSession(apkUri).await() in try\catch block.
Have you considered instead of throwing exceptions to pass typed errors instead as you already do, but for all other cases too? It helps with understanding what could happen and react to it before it happens.

For me as user of library I can't really distinguish between InstallFailure.Exceptional and Exception been thrown.
One example would be - I encountered FileNotExists exception (when apk file was missing), while I expected some sort of InstallFailure

[solrudev]

@nvkleban Thanks for an interesting question! This is quite complicated topic. In my opinion, as we're dealing with coroutines, await() function should rethrow all exceptions encountered, because this is an expected behavior by convention. Thus, actually, you'll never get InstallFailure.Exceptional as a return value.

However, as you noted, this is quite confusing. This peculiarity stems from core architectural choice of designing Session as an observable object to not be dependent on a concrete asynchrony implementation baked into frameworks or a language. I couldn't make Exceptional failure to not be a part of SessionResult.Error failure sealed hierarchy and instead to be a part of a separate one and maintain the same types for failures across modules. Sadly, I don't think Kotlin's type system allows this. Taking into consideration what I wrote in the first paragraph, I'm reluctant to introduce such a change. As a workaround, you can write your own await() wrapper which will catch exceptions and return them as InstallFailure.Exceptional objects, though I'll refrain from providing such functionality in the library.

Perhaps, the best compromise is to introduce a separate sealed hierarchy of failures specifically for coroutines, like I suggested above, though it'll be a major API change. What do you think?

[solrudev]

Now that I think about the last suggestion I made, I'm sure that it also won't work out. So, we can't omit a type Exceptional from failures sealed hierarchy for await(). That leaves us with a question, whether to wrap exceptions or not. My stance is outlined in previous comment. If you have arguments against, I'm ready to consider them.

[nvkleban]

There is couple of my thoughts on the subject. Take it with the grain of salt.

1. Are all current exceptions should be exceptions? A lot of them are expected to happen sometimes, and could be a part of the sealed hierarchy. For i.e. mentioned above FileNotExists.
2. Are exceptions that should not happen ever (unless there is a bug in library) should be delivered in Exceptional to user as part of this sealed hierarchy? I think no as this situation should never happen and if it happens - I would rather prefer crash (fail fast approach). For i.e. cases where you look for extras in your own intents.
3. If there any cases where exceptions are not fit into 2 previous categories. As a library user do I even need them? Like what I can do with it. It may be as well just a Generic failure.
4. Exceptions are generally discouraged in Kotlin, while suspend function may rethrow any encountered exceptions CancelationExceptions, OOM, logic bugs etc. it doesn't inherently means that all other errors should be exceptions.

There is an excellent read for Roman Elizarov about exceptions in Kotlin

As a rule of thumb, you should not be catching exceptions in general Kotlin code. That’s a code smell. Exceptions should be handled by some top-level framework code of your application to alert developers of the bugs in the code and to restart your application or its affected operation. That’s the primary purpose of exceptions in Kotlin.

Thus, the same general advice applies to exceptions and coroutines: don’t use exceptions if you need local handling of certain failure scenarios in your code, don’t use exceptions to return a result value, avoid try/catch in general application code, implement centralized exception-handling logic, handle input/output errors uniformly at an appropriate boundary of your code.

P.S. It's totally not a blocker as I use the library like you mentioned in a doc, wrapping it in a try\catch, just a genuine question that appeared in my head while I was using it.

[solrudev]

@nvkleban
So, in general we share mostly the same view.

1. All expected failures are handled with the existing sealed hierarchy, and unless there's faulty input, or bug in the library, or factors not controlled by it come into play, they should fall into those. To break down your example, Ackpine doesn't do file existence checks as it expects that checks are not needed (e.g. user input from file picker or freshly created file) or were made by library consumer, hereby FileNotFoundException is viewed as an exceptional outcome. I'm not even sure where it's being thrown. Well, now it may become a discussion about whether the library should check inputs, but if it will, then these expected failures would be wrapped as Generic.
2. I agree. Like I said, I view all exceptions originating in the library as unexpected ones. For example, I/O operations in general case are always expected to complete successfully. That's why, as I mentioned earlier, exceptions will never be delivered as Exceptional failures from await(), they will be thrown. There's some inconvenience because of that though when matching on a returned failure type, but I can't find an elegant solution. Maybe this is worth mentioning in the docs.
3. I don't think there are cases like this.
4. See above.