Some Async Coroutine Functions #431

iNoles · 2018-09-14T01:50:10Z

Thank you for submitting your Pull Request. Please make sure you have
familiarised yourself with the Contributing Guidelines
before continuing.

Description

I added async block for some functions and left blocking one alone. I was a bit curious why somebody else didn't added this one.

Fixes #429

Type of change

Check all that apply

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactoring (change which changes the current internal or external interface)
This change requires a documentation update

How Has This Been Tested?

In case you did not include tests describe why you and how you have verified the
changes, with instructions so we can reproduce. If you have added comprehensive
tests for your changes, you may ommit this section.

Checklist:

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation, if necessary
My changes generate no new compiler warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes

SleeplessByte · 2018-09-14T02:13:32Z

I'm personally not a fan of the double await. asyncObjectResponse().await() would work, and allows you to have this be additive and not breaking.

iNoles · 2018-09-14T02:15:40Z

@SleeplessByte Yeah, I see what you mean.

Added Async coroutine functions

markGilchrist · 2018-09-14T06:56:30Z

fuel-coroutines/src/main/kotlin/com/github/kittinunf/fuel/coroutines/Coroutines.kt

-import kotlinx.coroutines.experimental.CommonPool
-import kotlinx.coroutines.experimental.suspendCancellableCoroutine
-import kotlinx.coroutines.experimental.withContext
+import kotlinx.coroutines.experimental.*


would you mind reverting this change as we try not to use wildcard imports

Okay, I will.

markGilchrist · 2018-09-14T06:58:34Z

fuel-coroutines/src/main/kotlin/com/github/kittinunf/fuel/coroutines/Coroutines.kt

-        await(byteArrayDeserializer(), scope)
+): Triple<Request, Response, Result<ByteArray, FuelError>> = await(byteArrayDeserializer(), scope)
+
+suspend fun Request.asyncByteArrayResponse(


is there a reason for changing the name of this function?

It looks like await on next line.

sorry I see that

kittinunf · 2018-09-15T08:58:41Z

fuel-coroutines/src/test/kotlin/com/github/kittinunf/fuel/coroutines/CoroutinesTest.kt

@@ -23,7 +23,7 @@ class CoroutinesTest {
    @Test
    fun testAwaitResponseSuccess() = runBlocking {
        try {
-            Fuel.get("/ip").awaitByteArrayResponse().third.fold({ data ->


Don't we want to keep awaitXX version for our user to use? Are we gonna deprecated them and move user to use the asyncXX version instead? I think either way we probably want to keep the tests for awaitXXX

Yeah definitely keep the tests until the await versions are removed, if ever.

Oh Okay, I will restore awaitXX versions of tests.

kittinunf · 2018-09-15T17:04:01Z

I think this looks great. It almost like we should deprecate the awaitXX version and expose only asyncXX. Because it looks like expose it as a Deferred triumphs other versions in every way 💦

All in all, after this PR is merged our Coroutine API landscape will look like this.

Type\Return	*	Result<*, FuelError>	`Triple<Request, Response, Result<*, FuelError>`	`Deferred<Triple<Request, Response, Result<*, FuelError>>>`
ByteArray	`awaitByteArray`	`awaitByteArrayResult`	`awaitByteArrayResponse`	`asyncByteArrayResponse`
String	`awaitString`	`awaitStringResult`	`awaitStringResponse`	`asyncStringResponse`
T	`awaitObject`	`awaitObjectResult`	`awaitObjectResponse`	`asyncObjectResponse`

It looks a bit wild.

I think it might be "good" to provide the same treatment (provide the Deferred<> version) with the rest of the available API. 😅

But this means that we are going to double the available API for our users. .... 🙀

I almost feel like, at this point, we might wanna clean this all up at once. Might it be just wiser to return the Deferred<> version instead?

What do you think?

iNoles · 2018-09-15T18:10:58Z

@kittinunf I would love to deprecated Result and Triple and only uses Deferred.

SleeplessByte · 2018-09-15T20:58:22Z

I concur with Jonathan: Deprecate, don't remove, but remove from documentation.

…

On Sat, 15 Sep 2018, 20:10 Jonathan, ***@***.***> wrote: @kittinunf <https://github.com/kittinunf> I would love to deprecated Result and Triple and only uses Deferred. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#431 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB35WLP0QfL5BOB8kkKpOT4-gUWNFs-9ks5ubUKzgaJpZM4Woevr> .

kittinunf · 2018-09-16T05:42:12Z

Just to confirm, I guess we want to go with just 2 versions!

Type\Return	Deferred<Result<*, FuelError>>	Deferred<Triple<Request, Response, Result<*, FuelError>>>
ByteArray	`asyncByteArrayResult`	`asyncByteArrayResponse`
String	`asyncStringResult`	`asyncStringResponse`
T	`asyncObjectResult`	`asyncObjectResponse`

And the rest, we shall deprecate them?

iNoles · 2018-09-16T14:22:10Z

@kittinunf
that would be like

async {
asyncByteArrayResponse().await().third
}

You don't need to have async on that line.

kittinunf · 2018-09-17T03:13:35Z

Hmm, in my experience, Result<*, FuelError> is actually being used around 80% of my time, the rest about 20%, I need to check for HTTP status code, that is where the Triple version becomes handy.

@iNoles If you don't mind, could you help me out?

iNoles · 2018-09-17T03:47:30Z

@kittinunf Sure
I would go this route

fun awaitByteArrayResult(): Result<ByteArray, FuelError> = asyncByteArrayResponse().await().third

kittinunf · 2018-09-17T08:07:30Z

Hmm, I think it is supposed to be Deferred<Result<T, FuelError>> ...

This is for the parity of the APIs so that asyncXXXResponse return Deferred<Triple<>> while asyncXXXResult return Deferred<Result<>>

Do you think it makes sense?

SleeplessByte · 2018-09-17T14:52:31Z

The await version is not deferred because doing an . await blocks. So the async one is deferred. Seems correct?

…

On Mon, 17 Sep 2018, 10:07 Kittinun Vantasin, ***@***.***> wrote: Hmm, I think it is supposed to be Deferred<Result<T, FuelError>> ... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#431 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB35WCg-QABUM1dCutdxr69r30NmNcuQks5ub1hDgaJpZM4Woevr> .

iNoles · 2018-09-17T15:08:17Z

@SleeplessByte Deferred's await() will wait for Deferred to complete.

@kittinunf I have a concern about it over asyncXXXResult because it is adding more complexity to the developers. It is going to be like asyncXXXResponse().await().third.await(). It going to be Confusing for double await().

SleeplessByte · 2018-09-17T15:55:06Z

@iNoles exactly what I thought :) And I agree with the double await being confusing.

clarfonthey · 2018-09-18T04:42:04Z

Using a Deferred doesn't actually solve this problem, sadly. It also seems like an anti-pattern because it's easy to just wrap the await in your own async call, whereas this forces you to use the Deferred, which is dispatched on a context you can't control.

The main problem described in #429 is that the coroutine still blocks instead of suspending the thread when it's waiting on I/O. This doesn't actually fix that problem.

iNoles · 2018-09-18T06:13:02Z

Using a Deferred doesn't actually solve this problem, sadly. It also seems like an anti-pattern because it's easy to just wrap the await in your own async call, whereas this forces you to use the Deferred, which is dispatched on a context you can't control.

The main problem described in #429 is that the coroutine still blocks instead of suspending the thread when it's waiting on I/O. This doesn't actually fix that problem.

Oh, what does solve this problem?

markGilchrist · 2018-09-18T07:22:53Z

@clarcharr @SleeplessByte I am sure that I may well have this wrong, however when I first looked at implementing this I can remember that I felt the best approach to make this a 'true' suspending implementation one would have to change (or implement similar)

line 53, Deserializable.kt

private fun <T : Any, U : Deserializable<T>> Request.response(deserializable: U,
                                                              success: (Request, Response, T) -> Unit,
                                                              failure: (Request, Response, FuelError) -> Unit): Request {
...

as this (if I understand this correctly) is the part that makes the request.

I think that one would have to add a suspending function here that executes the task in order to achieve what you are looking for

kittinunf · 2018-09-18T09:05:57Z

Ahhhh sorry, I understand it now. Then, the current API proposal is OK to me.

@clarcharr I think we need you to perhaps shed some light here. I think the last thing we want is merging this and it doesn't solve your problem.

iNoles · 2018-09-18T14:43:16Z

@kittinunf Do you want me to restore

fun awaitByteArrayResult(): Result<ByteArray, FuelError> = asyncByteArrayResponse().await().third

because 80% of the time?

kittinunf · 2018-09-19T15:05:22Z

I feel like if I am gonna use the coroutine version, I think I might prefer to use Deferred<Result<T, E> instead. Because I don't use coroutine on my work at all so please take my opinion for a gain of salt.

clarfonthey · 2018-09-21T15:30:33Z

I'm not going to be able to get to this today, but I'll try this weekend to draft up the sort of changes that would be needed to fix the original issue. Part of it can be done without really affecting the surface API, but another part can't.

Basically, I think that establishing a connection to the server and getting back the response can be done asynchronously, where the only difference from the existing implementation is that the current thread isn't blocked from performing other work while the connection happens.

Making the read of the response body asynchronous would require changing the deserialization API, which would be breaking. But as far as things go, a majority of the time for the request is in getting back the response headers, which means that there's still a lot to gain without breaking the API.

markGilchrist · 2018-09-21T17:17:44Z

@SleeplessByte @iNoles @clarcharr I would really value your critique on my pr #438

lucasvsme · 2018-09-23T21:02:12Z

Just a note about returning a Deferred<T>: https://stackoverflow.com/questions/49762153/modify-the-deferred-result/49762498#49762498

codecov · 2018-09-29T14:56:19Z

Codecov Report

Merging #431 into master will decrease coverage by 0.99%.
The diff coverage is 0%.

@@           Coverage Diff            @@
##             master    #431   +/-   ##
========================================
- Coverage      77.8%   76.8%   -1%     
  Complexity      164     164           
========================================
  Files            33      33           
  Lines           811     815    +4     
  Branches        129     135    +6     
========================================
- Hits            631     626    -5     
- Misses          114     129   +15     
+ Partials         66      60    -6

Impacted Files	Coverage Δ	Complexity Δ
...com/github/kittinunf/fuel/coroutines/Coroutines.kt	`9.67% <0%> (-19.96%)`	`0 <0> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3393d38...c7d6291. Read the comment docs.

iNoles · 2018-10-04T17:28:30Z

I am going to close this PR because async-await is BEST for Multiple coroutines with concurrency. Since 80% of the cases, it going to be single operation that is best for withContext.

iNoles changed the title ~~Some coroutine functions are under async now.~~ Some Async Coroutine Functions Sep 14, 2018

Some coroutine functions are under async now.

d12aa86

Added Async coroutine functions

markGilchrist reviewed Sep 14, 2018

View reviewed changes

Optimize Imports

f038e28

kittinunf reviewed Sep 15, 2018

View reviewed changes

Restore AwaitXX versions of tests and rename some function to Async**.

286f5f8

SleeplessByte mentioned this pull request Sep 15, 2018

Actually asynchronous coroutines #429

Closed

Deprecated all await** versions.

4b84ffb

Merge branch 'master' into AsyncCoroutines

e73bcb0

markGilchrist mentioned this pull request Sep 24, 2018

Add suspending implenetation client #438

Merged

11 tasks

kittinunf added the ⚠️ work in progress Under construction. Do not merge. This is not final. label Sep 26, 2018

Merge branch 'master' into AsyncCoroutines

40c52ce

iNoles and others added 3 commits September 29, 2018 22:51

Fixed tests after merged

4e6a369

Fixed Samples

59cb954

Merge branch 'master' into AsyncCoroutines

a26945c

iNoles self-assigned this Sep 30, 2018

Merge branch 'master' into AsyncCoroutines

c7d6291

iNoles closed this Oct 4, 2018

iNoles deleted the AsyncCoroutines branch October 4, 2018 17:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some Async Coroutine Functions #431

Some Async Coroutine Functions #431

iNoles commented Sep 14, 2018 •

edited

SleeplessByte commented Sep 14, 2018

iNoles commented Sep 14, 2018 •

edited

markGilchrist Sep 14, 2018

iNoles Sep 14, 2018

markGilchrist Sep 14, 2018

iNoles Sep 14, 2018 •

edited

markGilchrist Sep 14, 2018

kittinunf Sep 15, 2018

SleeplessByte Sep 15, 2018

iNoles Sep 15, 2018

kittinunf commented Sep 15, 2018 •

edited

iNoles commented Sep 15, 2018

SleeplessByte commented Sep 15, 2018 via email

kittinunf commented Sep 16, 2018 •

edited

iNoles commented Sep 16, 2018

kittinunf commented Sep 17, 2018

iNoles commented Sep 17, 2018

kittinunf commented Sep 17, 2018 •

edited

SleeplessByte commented Sep 17, 2018 via email

iNoles commented Sep 17, 2018

SleeplessByte commented Sep 17, 2018

clarfonthey commented Sep 18, 2018

iNoles commented Sep 18, 2018

markGilchrist commented Sep 18, 2018

kittinunf commented Sep 18, 2018

iNoles commented Sep 18, 2018

kittinunf commented Sep 19, 2018 •

edited

clarfonthey commented Sep 21, 2018

markGilchrist commented Sep 21, 2018

lucasvsme commented Sep 23, 2018

codecov bot commented Sep 29, 2018 •

edited

iNoles commented Oct 4, 2018

Some Async Coroutine Functions #431

Some Async Coroutine Functions #431

Conversation

iNoles commented Sep 14, 2018 • edited

Description

Type of change

How Has This Been Tested?

Checklist:

SleeplessByte commented Sep 14, 2018

iNoles commented Sep 14, 2018 • edited

markGilchrist Sep 14, 2018

Choose a reason for hiding this comment

iNoles Sep 14, 2018

Choose a reason for hiding this comment

markGilchrist Sep 14, 2018

Choose a reason for hiding this comment

iNoles Sep 14, 2018 • edited

Choose a reason for hiding this comment

markGilchrist Sep 14, 2018

Choose a reason for hiding this comment

kittinunf Sep 15, 2018

Choose a reason for hiding this comment

SleeplessByte Sep 15, 2018

Choose a reason for hiding this comment

iNoles Sep 15, 2018

Choose a reason for hiding this comment

kittinunf commented Sep 15, 2018 • edited

iNoles commented Sep 15, 2018

SleeplessByte commented Sep 15, 2018 via email

kittinunf commented Sep 16, 2018 • edited

iNoles commented Sep 16, 2018

kittinunf commented Sep 17, 2018

iNoles commented Sep 17, 2018

kittinunf commented Sep 17, 2018 • edited

SleeplessByte commented Sep 17, 2018 via email

iNoles commented Sep 17, 2018

SleeplessByte commented Sep 17, 2018

clarfonthey commented Sep 18, 2018

iNoles commented Sep 18, 2018

markGilchrist commented Sep 18, 2018

kittinunf commented Sep 18, 2018

iNoles commented Sep 18, 2018

kittinunf commented Sep 19, 2018 • edited

clarfonthey commented Sep 21, 2018

markGilchrist commented Sep 21, 2018

lucasvsme commented Sep 23, 2018

codecov bot commented Sep 29, 2018 • edited

Codecov Report

iNoles commented Oct 4, 2018

iNoles commented Sep 14, 2018 •

edited

iNoles commented Sep 14, 2018 •

edited

iNoles Sep 14, 2018 •

edited

kittinunf commented Sep 15, 2018 •

edited

kittinunf commented Sep 16, 2018 •

edited

kittinunf commented Sep 17, 2018 •

edited

kittinunf commented Sep 19, 2018 •

edited

codecov bot commented Sep 29, 2018 •

edited