Fuel: Coroutine-variant is throwing while blocking equivalent is not

Created on 12 Jun 2018 · 6Comments · Source: kittinunf/fuel

I'm implementing some kind of custom caching. Because of this 304 is an interesting response for me. When getting the Triple from .response() I correctly get a Result, which is a failure. When using .awaitResponse() the continuation is throwing instead. Of course I could try-catch the FuelError, but I believe changing Fuels behaviour so that the coroutine-case matches the default case makes more sense.

My code looks like this:

````kotlin
class CachedHttpLineSource(private val prefs: Prefs, private val uri: String, private val userName: String, private val password: String, private val cacheDir: File) : LineSource() {
@Serializable
data class CachedFile(val uri: String, val etag: String?, val savedPath: String)

private fun getDownloadRequest(etag: String? = null): Request {
    val req = Fuel.download(uri).authenticate(userName, password)
    if (etag == null) return req
    return req.header("If-None-Match" to etag)
}

private suspend fun download(etag: String? = null): CachedFile {
    val file = File(cacheDir, uri.sha1())
    if (file.exists()) file.delete()
    val triple = getDownloadRequest(etag).destination { _, _ -> file }.awaitResponse() // does throw here
    val response = triple.second
    val result =  triple.third
    when (result) {
        is Result.Failure -> { throw(result.getException()) } // should throw here
        is Result.Success -> { return CachedFile(uri, response.headers["ETag"]?.firstOrNull(), file.absolutePath) }
    }
}

}
````

Looking into Coroutines.kt reveals the problem. Using fold for all cases instead of just returning the Triple seems wrong. As far as I understand it could work to just remove the folding and resume with the Triple in any case as the .third.get() should throw the exception in the relevant cases too.

````kotlin
// line 23
response(deserializable) { request: Request, response: Response, result: Result ->
result.fold({
continuation.resume(Triple(request, response, result))
}, {
continuation.resumeWithException(it.exception)
})
}

// line 32
suspend fun Request.awaitResponse(): Triple> =
await(byteArrayDeserializer())

// line 43
suspend fun Request.awaitResponseResult(): ByteArray = awaitResponse().third.get()
````

bug

Source

georg-jung

Most helpful comment

@georg-jung The pull request #373 has been merged and should solve this issue.

lucasvalenteds on 7 Jul 2018

👍2

All 6 comments

Thank your for the feedback, @georg-jung.

So if I understand correctly the API may still provide two options of handling failures. The methods ending with Result should supposed to be used with try-catch and the others should be handled like the "standard" API. Something like below.

val responseA = Fuel.get("/error/404").response() // Does not throw
val responseB = Fuel.get("/error/404").awaitResponse() // Does not throw

try {
    val responseC = Fuel.get("/error/404").awaitResponseResult() // Throw
} catch(error: FuelError) {
    println(error.message)
}

I've changed the code in a branch to experiment with this idea and it seems to work (all tests passing). This also may be related to #358.

lucasvalenteds on 12 Jun 2018

👍2

@lucasvalenteds This is the issue I was referencing in #358

.awaitResponse() does throw whereas .response() does not

There is currently only 1 unit test on .awaitResponse() in CoroutinesTest.kt and it's verifying that when there's a result everything works as expected. If you add the below test, you will see the failure

try {
            val (_, response, result) = Fuel.get("/error/404").awaitResponse()
            assertTrue(response.statusCode == 404)
            assertTrue(result.getOrElse(ByteArray(0)).isEmpty())
        } catch (ex: HttpException) {
            fail("This test should not throw an exception")
        }