Handle fetchers that don't throw an exception

[ebrowne72]

Exceptions aren't good. I think the Kotlin community is coalescing around returning a sealed class with error information instead of throwing an exception to indicate that a function has an error. That's what Store does with the StoreResponse class. However, a StoreResponse.Error object will only be returned if the fetcher throws an exception. Now the libraries commonly used by a fetcher, such as Retrofit or Ktor, throw exceptions on error (bad Ktor, no biscuit). But what if someone wants to use a library that returns a sealed class to indicate success or failure?

For example, if MyLibrary returns a MyLibraryResult.Success or a MyLibraryResult.Error, how should the fetcher or Store handle it?

• The app could define a custom exception which the fetcher throws when it receives an error, but we're trying to get away from exceptions. Introducing an exception between two libraries that don't use exceptions doesn't sound right.
• The fetcher could return the MyLibraryResult object (whether or not there was an error), but then Store doesn't know there was a failure. Also the persister has to deal with unwrapping the data while storing it, and handling any errors.
• There could be some direct way for the fetcher to tell Store there was a failure. The fetcher lambda could be an extension function on an interface, perhaps with a reportError() method. If this is called then the fetcher's return value is ignored and it is treated as an error.
• The fetcher returns a StoreResponse.Error object, which Store detects and treats the same as if an exception was thrown.
• The StoreBuilder functions, in addition to the key and output types, has a third type for errors. If the fetcher returns an object of this class, it treats it as an error. For example: StoreBuilder.fromNotFlow<String, Data, MyLibraryResult.Error>(). If the fetcher succeeds, it unwraps the data from the result class and returns it. If it fails, it returns an error object.

The StoreResponse class would need to be modified to handle this new kind of error. Maybe there are two error subclasses, one for exceptions and one for non-exceptions. Maybe StoreResponse.Error can contain either a Throwable or some other error class.

[digitalbuddha]

But what if someone wants to use a library that returns a sealed class to indicate success or failure? This is definitely not part of any design we had in mind, we tried to keep the api surface as small as possible while delegating most decisions to user, result type was not one we had in mind.

Out of the solutions offered I like the following one the best: The fetcher returns a StoreResponse.Error object, which Store detects and treats the same as if an exception was thrown.

This is actually closer to how store3 worked due to Rxjava being able to make Observable.error() rather than throwing an error.

Let's see what others think but to me that sounds like a reasonable solution of allowing fetcher to return raw values or ones wrapped in store response that we unwrap.

Thank you for the detailed explanation and suggestions for fixes!

[eyalgu]

One additional alternative is that the builder can accept an additional parameter: fetchResultDecoder: (Input) -> StoreResult<UnwrappedInput> (similar to adding a source of truth, this will change the Value type of the resulting store).

The benefits of this approach is:

1. It's more discoverable than updating the documentation of fetcher to say that, if it returns StoreResult.Error it will be treated differently.
2. It doesn't force the user to use StoreResult as Input for their sourceOfTruth, or worse yet, as the Value of the resulting store if there's no source of truth.

The main downside is that we're adding another concept to the API, but given that it's an optional parameter in the builder that might not be too bad.

Thoughts?

[yigit]

I think it still makes sense to default to exception as many people will provide a fetcher that uses retrofit and it does throw exceptions when the function is a suspend function.

I'm not against providing the alternative though i'm not sure if the added API cost is worth the benefit as from the developer's perspective, all they need is a wrapper that'll throw if the return type is error.

[digitalbuddha]

My take is if something can be done with a wrapper without too much trouble then no reason to support it out of the box. Ideally we keep our api small and pluggable

[eyalgu]

I think that what @ebrowne72 is asking for is to be able to handle network errors without throwing exceptions which we currently don't support.

As @ebrowne72 points out though, currently the popular networking libraries don't support it so it's not going to be useful for most developers.

My take is let's track interest on this issue. If enough people™ ask for it we would add support

[ebrowne72]

As @yigit said, users can use a wrapper to throw an exception. But this means if I have a fetcher which calls something which returns an error object instead of throwing an exception, then I have to define an exception in my app, instantiate that exception, and then throw it. And since I think exceptions aren't good, making me define and throw an exception to indicate failure doesn't feel right. That isn't a solution, it's a recommended workaround.

This isn't a problem now, since the libraries most (all?) fetchers will use today date from the "exception era". But I'd hate to have someone ask on StackOverflow in a few years how to tell Store a fetcher failed when there's no exception.

[hansenji]

It seems contradictory that store returns and error state but only allows that error state to be triggered by exceptions. Exceptions should be exceptional. In mobile a failed connection due to network disconnecting is not exceptional behavior. One just has to go on a bus or train ride to watch the network go up and down multiple times.

[eyalgu]

I personally don't think it's too big of a deal to add support for that, but I'm a relatively new contributor to the project so going to defer to @yigit and @digitalbuddha. IMO we can always throw an experimental annotation on it and remove support if we decide it's not worth the maintenance cost

[yigit]

I don't think in the world of Kotlin Flow, exceptions are "exceptional". In fact, the whole flow works on exceptions as a cancellation mechanism. Exceptions are the only way to stop a collections, e.g. this is the implementation of flow.first(): https://github.com/Kotlin/kotlinx.coroutines/blob/master/kotlinx-coroutines-core/common/src/flow/terminal/Reduce.kt#L88

public suspend fun <T> Flow<T>.first(): T {
    var result: Any? = NULL
    try {
        collect { value ->
            result = value
            throw AbortFlowException(NopCollector)
        }
    } catch (e: AbortFlowException) {
        // Do nothing
    }

    if (result === NULL) throw NoSuchElementException("Expected at least one element")
    return result as T
}

As for why Store does not throw when an error happens is because we do not want to stop the stream when an error happens. The practical use case is data driven UIs. When you have some code like this:

store.stream(StoreRequest.cached(userId, refresh=true)).collect {
  // update UI
}

you don't want stream to be cancelled just because network request failed. It should still send new values if data is fetched for another request or simply because data changed on disk. So from Store's perspective, the fact that fetch failed does not mean an error to cancel the stream, it only is an event that denotes the attempt to refresh has failed. Of course, if the desired behavior is to cancel it, you can always apply a transformation or use one of the methods that flow data instead of events.

[yigit]

are there any more comments on this or should we close this bug ?

[hansenji]

Exceptions being used for control flow causes performance issues that can easily be avoided by verifying conditions. Gathering the stack trace for a network not connected exception compared to just checking if the network is available is an order of magnitude slower. Retrofit supports returning a Response object so it won't throw an exception if the call fails and it improves visibility into the network call.

I love that Store uses sealed classes to not cancel flows when network requests fail it is one of its selling points to me.

[hansenji]

It is not uncommon for a library to require an implementation to return a specific result type. WorkManager for example has a response type result with a success and failed state. Granted WorkManager result class has a third state of retry. However, supporting such a response or result type, especially a sealed class, allows this performance hit to be avoided and can still allow exceptions to be caught and turned into an error.

[eyalgu]

I'm sympathetic to this request TBH. We, as part of Store's API, return an Error when the fetcher fails. I think it makes sense to allow the fetcher to trigger the error state without throwing an exception. That way we support the same API we expose.

[yigit]

We are discussing a similar case for Paging 3 in AndroidX and we are leaning towards returning a Result/Error types instead of thrown exceptions. tl;dr; of that conversation was (in case of retrofit): if you return response rather than value, RF will throw an exception if you've configured it wrong but will return response if a reasonable error happened (like IO error). I personally still like returning value but I'm happy to agree to change them to return store response. It will be a bit weird for source of truth though. Maybe we can make fetchers return store response but source of truth just return values? I'm not sure. Supporting both for fetcher seems like an API challange so i'm not very keen on that option unless we can find a nice API that is not confusing.

[eyalgu]

I think an optional convertor in the builder is probably the cleanest way to support that. I'll try to throw up a quick pr this weekend so we can discuss on it

On Sat, Feb 29, 2020, 10:23 AM Yigit Boyar notifications@github.com wrote:

We are discussing a similar case for Paging 3 in AndroidX and we are leaning towards returning a Result/Error types instead of thrown exceptions. tl;dr; of that conversation was (in case of retrofit): if you return response rather than value, RF will throw an exception if you've configured it wrong but will return response if a reasonable error happened (like IO error). I personally still like returning value but I'm happy to agree to change them to return store response. It will be a bit weird for source of truth though. Maybe we can make fetchers return store response but source of truth just return values? I'm not sure. Supporting both for fetcher seems like an API challange so i'm not very keen on that option unless we can find a nice API that is not confusing.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/dropbox/Store/issues/82?email_source=notifications&email_token=AAMUYKPBFET327ILVIREXSDRFFJC7A5CNFSM4KM4V5BKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENMBYMQ#issuecomment-592976946, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMUYKNR5XBCWYBWVJRFWX3RFFJC7ANCNFSM4KM4V5BA .

[eyalgu]

see #123 as an example of what I mean. it's not a working PR but should give you a sense of the scope of the change

[yigit]

What if we just changed it to always return a FetcherResult and ditch support for T. Looks like that is what we are leaning towards in paging. Even thought that API is uglier, catching exceptions is also problematic as it might be an unexpected exception instead of something like IO.

[eyalgu]

What would we do in case of exceptions then? right now we never terminate the flow from store, would that change?

I'm a little hastiest about only supporting FetcherResult because it would make integration with Retrofit harder which is the most common usecase

[yigit]

maybe we can figure out some integration w/ retrofit. See retrofit case in https://github.com/dropbox/Store/issues/82#issuecomment-592976946 tl;dr; using the retrofit API that returns values along w/ Store means ignoring all kinds of exceptions, not just network connectivity issues. And if we wrap it, we won't even know if they just get an NPE in the fetcher which is not great (that is the kind of case when you want your app to crash).

To be very clear, I'm still "camp return direct value". But i guess there is some value in being consistent w/ other things :man_shrugging: And supporting both seems to be a real API challenge.