Fuel Coroutines Failing on Android with NetworkOnMainThreadException after upgrade

[avaitla]

Hello Team,

I have encountered issues trying to upgrade my fuel version in my Android App, and would like to see if this is related to the fuel/coroutines package or to my own incorrect usage of coroutines within android.

The app worked great but now breaks with an android.os.NetworkOnMainThreadException when I make the following changes:

1. Fuel Version Bump: v1.13.0 -> v1.14.0
2. Coroutines Version Bump: v0.22.5 -> v0.23.4
3. Replace `awaitObject` with `awaitObjectResponse`

I've created a simple repository to reproduce the issue here: https://github.com/avaitla/coroutinesissue
It has two branches:

good_v1.13.0 - uses fuel v1.13.0 and coroutines v0.22.5
broken_v1.14.0 - uses fuel v1.14.0 and coroutines v0.23.4

See the diff that breaks things here:

https://github.com/avaitla/coroutinesissue/commit/cba42a68498b217ce48dfdbc1abf7d279558d96f

The main activity class is here:

https://github.com/avaitla/coroutinesissue/blob/good_v1.13.0/app/src/main/java/com/avaitla16/coroutinesissue/MainActivity.kt

I've attached a screen shot from the good_version branch:

Here is the screen shot from the bad_version branch:

Thanks for your time!

[avaitla]

Given the following:

data class UUID(val uuid: String)

suspend fun getData(): Result<UUID, FuelError> {
    val awaitObject = "https://httpbin.org/uuid".httpGet().awaitObjectResponse (gsonDeserializerOf<UUID>())
    return awaitObject.third
}

I have noticed that replacing the old code here:

fun makeRequest() = launch(UI) {
    val result = getData()

    when (result) {
        is Result.Failure -> {
            val ex = result.getException()
            Toast.makeText(this@MainActivity, "Failure: $ex", Toast.LENGTH_LONG).show()
        }

        is Result.Success -> {
            val data = result.get()
            Toast.makeText(this@MainActivity, "Success: $data", Toast.LENGTH_LONG).show()
        }
    }
}

With the following (delegating to the commonpool) allows the app to work:

fun makeRequest() = {
    val job = async(CommonPool) { getData() }

    launch(UI) {
        val result = job.await()
        when (result) {
            is Result.Failure -> {
                val ex = result.getException()
                Toast.makeText(this@MainActivity, "Failure: $ex", Toast.LENGTH_LONG).show()
            }

            is Result.Success -> {
                val data = result.get()
                Toast.makeText(this@MainActivity, "Success: $data", Toast.LENGTH_LONG).show()
            }
        }
    }
}

Is this the appropriate way to use coroutines / fuel api within an android app? Initially it does feel like more boilerplate than before.

[markGilchrist]

@avaitla thank you for taking the time to raise such a well written issue, I am looking at this now

[markGilchrist]

@avaitla I have raised a PR to resolve this #387

thank you once again for taking the time to raise such a well written issue.

I am sorry for any inconvenience that has been caused

[avaitla]

Thank you @markGilchrist. This change will make our async code much cleaner to write as the caller will not need to switch between two async pools.

[raharrison]

I don't think this is the way to go. Even though it might seem like a bit more code, explicitly defining the pool and then switching to the UI thread follows the overall coroutine philosophy - a.k.a concurrency is explicit.

Point being that the library is now enforcing the usage of CommonPool to perform async operations, this isn't something that should be abstracted away as from a usage perspective the developer should know what pools are being utilised at all times - especially for blocking operations. In larger apps it's highly likely that a dedicated pool will be used so there would need to be some way of overriding this.

This is somewhat of a redundant issue in the case of Fuel because it uses another thread pool behind the scenes anyway (although this should also probably be rethought as was previously discussed when coroutine support was originally added). However, following the general design pattern is something we should maintain.

You can also simplify your code anyway by returning a Deferred from getData and reusing the older code:

fun getData(): Deferred<Result<UUID, FuelError>> = async(CommonPool) {
    "https://httpbin.org/uuid".httpGet().awaitObjectResponse (gsonDeserializerOf<UUID>()).third
}

[markGilchrist]

@raharrison I hear what you say and I think that you have raised a very good point

would you think that it would be suitable to add default arguments to all the coroutine functions such that the consumer of the library can specify which pool the function call is made on?

or do you feel that everyone should consume the code as per you comment above.

I would be happy to update my pr with an extra argument for the scope?

[raharrison]

I'm fine with adding default arguments with CommonPool if that helps people. Ultimately, as long as it's not hardcoded and configurable then I think we're going down the right route.

[markGilchrist]

@raharrison give me 20 mins to update the branch (I almost did this first time round) would you mind adding comments to the forth coming pr?

[markGilchrist]

#387

[avaitla]

Thanks for taking a look everyone. I agree about making sure the pool is configurable, but just wanted to add that in the simple common cases (this example), using coroutines feels overly difficult without a default configurable scope option in Fuel.

Without it, developers have to ensure they have their request method launched inside the common pool along with having the caller launch their own logic inside the ui pool. It is especially challenging the first time you have to figure this out since the test cases don't have an illustrative android example so you are somewhat on your own in this regard.

In comparison, for the simple cases, the callback style of fuel feels very pleasant to use since it handles the callback result on the android ui thread for users automatically.

[markGilchrist]

#387 is now merged, let me know when you are happy to close this issue?

[avaitla]

Thanks Mark. Please close it.