Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add first-party Kotlin coroutine suspend support #2886

Merged
merged 1 commit into from Feb 15, 2019

Conversation

@JakeWharton
Copy link
Collaborator

JakeWharton commented Sep 12, 2018

No description provided.

Show resolved Hide resolved pom.xml Outdated
if (requestFactory.isKotlinSuspendFunction) {
Type[] parameterTypes = method.getGenericParameterTypes();
Type continuationType = parameterTypes[parameterTypes.length - 1];
Type innerType = Utils.getParameterLowerBound(0, (ParameterizedType) continuationType);

This comment has been minimized.

@swankjesse

swankjesse Sep 16, 2018

Member

I don't love this name, just cause inner often means nested and not parameter. Maybe call it asyncResponseType?

This comment has been minimized.

@JakeWharton

JakeWharton Sep 16, 2018

Author Collaborator

Fixed by just using responseType which cleaned this section up a bit

@@ -32,8 +35,28 @@
*/
static <ResponseT, ReturnT> HttpServiceMethod<ResponseT, ReturnT> parseAnnotations(
Retrofit retrofit, Method method, RequestFactory requestFactory) {
CallAdapter<ResponseT, ReturnT> callAdapter = createCallAdapter(retrofit, method);
Type responseType = callAdapter.responseType();
CallAdapter<ResponseT, ReturnT> callAdapter = null;

This comment has been minimized.

@swankjesse

swankjesse Sep 16, 2018

Member

Just a wrinkle: asymmetry on how this is set up feels awkward. No suggestions on how to do it better.

This comment has been minimized.

@JakeWharton

JakeWharton Sep 16, 2018

Author Collaborator

Yeah I'm willing to muck up the internals instead of inventing some elaborate abstraction that will only be used by coroutines. If we ever need to generalize we can, but I don't want to jump that gun.

This comment has been minimized.

@JakeWharton

JakeWharton Feb 15, 2019

Author Collaborator

I've refined this a bit by using subtypes. This method is still gnarly because of all the shared logic, but the created classes are small and focused and don't involve crazy conditionals

This comment has been minimized.

@JakeWharton

JakeWharton Feb 15, 2019

Author Collaborator

This code actually reminds me of Retrofit internals circa 2012-2013... it can be cleaned up it's just a matter of effort but since it's isolated here for now I'm not worried about it.

Object continuation = args[args.length - 1];
if (continuationWantsResponse) {
//noinspection unchecked Guaranteed by parseAnnotations above.
return (ReturnT) KotlinExtensions.awaitResponse(call,

This comment has been minimized.

@swankjesse

swankjesse Sep 16, 2018

Member

This function name looks like it’ll be blocking but my assumption is that it'll return a promise. I gotta read more.

This comment has been minimized.

@JakeWharton

JakeWharton Sep 16, 2018

Author Collaborator

It actually returns a union type of COROUTINE_SUSPENDED | T which is either a marker value indication suspension to unwind the stack or the result if it was able to be computed synchronously. Since we're invoking Call.enqueue it will always return COROUTINE_SUSPENDED.

inline fun <reified T> Retrofit.create(): T = create(T::class.java)

suspend fun <T : Any> Call<T>.await(): T {

This comment has been minimized.

@swankjesse

swankjesse Sep 16, 2018

Member

This is neat.

This comment has been minimized.

@JakeWharton

JakeWharton Sep 16, 2018

Author Collaborator

And public API for people to use if they already have Calls! Makes it a win-win really.

enqueue(object : Callback<T> {
override fun onResponse(call: Call<T>, response: Response<T>) {
if (response.isSuccessful) {
// TODO handle nullability

This comment has been minimized.

@swankjesse

swankjesse Sep 16, 2018

Member

Presumably the callers T can itself be nullable or not – like the parameter will be String or String? but to this code it's all just T.

This comment has been minimized.

@JakeWharton

JakeWharton Sep 16, 2018

Author Collaborator

Which is a real problem in practice since an endpoint declared as suspend fun foo(): String can get an HTTP 204 which produces a null body and leaks that into an otherwise non-null type.

We need something like a hand-written Kotlin metadata parser for return type nullability that doesn't pull in any deps or support anything except looking for exactly what we need. That way it's 1 method and not 10,000.

@@ -98,6 +101,10 @@ static RequestFactory parseAnnotations(Retrofit retrofit, Method method) {
+ ") doesn't match expected count (" + handlers.length + ")");
}

if (isKotlinSuspendFunction) {
// The Continuation is the last parameter and the handlers array contains null at that index.

This comment has been minimized.

@swankjesse

swankjesse Sep 16, 2018

Member

Wondering aloud: could you symmetrically synthesize the return type from this argument? It’s how the coroutine and non-coroutine flows could fit together.

interface ResponseReturner<>T {
  void success (Response<T> response);
  void failure (Throwable t);
}

This comment has been minimized.

@swankjesse

swankjesse Sep 16, 2018

Member

I guess where I'm going is to imagine coroutines is actually a more general case and we can model regular returns within the coroutine machinery. Though this would only be for API symmetry.

This comment has been minimized.

@JakeWharton

JakeWharton Sep 16, 2018

Author Collaborator

What you're describing sounds like the callback factory that I originally modeled. It seems like a lot of work for something I expect no one except coroutines to use.

This comment has been minimized.

@swankjesse

swankjesse Sep 16, 2018

Member

Works for me! None of it leaks into the public API anyway so it doesn’t much matter.

One other way of potentially looking at this:

The coroutines support synthesizes a return type of Call<T> or Call<Response<T>> and just calls await() for you

isKotlinSuspendFunction = true;
return null;
}
} catch (NoClassDefFoundError ignored) {

This comment has been minimized.

@swankjesse

swankjesse Sep 16, 2018

Member

Sneaky!

Show resolved Hide resolved retrofit/src/main/java/retrofit2/Utils.java Outdated

/**
* This code path can only be tested from Java because Kotlin does not allow you specify a raw
* Response type. Win! We still test this codepath for completeness.

This comment has been minimized.

@swankjesse
@swankjesse
Copy link
Member

swankjesse left a comment

This is wonderful.

Is there an easy way to test that Retrofit works without the optional dependencies? Would help to defend against accidental introductions of Kotlin dependencies.


server.enqueue(MockResponse().setResponseCode(404))

try {

This comment has been minimized.

@swankjesse

swankjesse Sep 16, 2018

Member

kotlin.test.assertFailsWith !

This comment has been minimized.

@JakeWharton

JakeWharton Feb 15, 2019

Author Collaborator

Requires an extra dependency. Maybe i'll upgrade to JUnit 4.13 beta in another PR which has assertThrows

Show resolved Hide resolved retrofit/src/test/java/retrofit2/KotlinSuspendTest.kt
@JakeWharton

This comment has been minimized.

Copy link
Collaborator Author

JakeWharton commented Sep 16, 2018

The optional dependencies are implicitly tested by the other modules which do not include them. For example, if the instanceof check for Continuation wasn't behind a try/catch you would see it throw NCDFE in all of the converter and adapter code.

@JakeWharton JakeWharton force-pushed the jakew/suspend/2018-09-11 branch 2 times, most recently from d325a9e to 77af488 Sep 16, 2018

@swankjesse

This comment has been minimized.

Copy link
Member

swankjesse commented Sep 16, 2018

Thumbs up. The implicit coverage is good but I still sort of want to add something small and explicit – a test that asserts Kotlin isn't on the classpath and then does some Retrofit stuff. That way if/when we add indirect Kotlin dependencies (such as via Okio 2!) then we have something to catch if we regress.

@bolein

bolein approved these changes Oct 19, 2018

@adriancole
Copy link
Contributor

adriancole left a comment

fantastic

@FabianTerhorst

This comment has been minimized.

Copy link

FabianTerhorst commented Oct 29, 2018

kotlin 1.3 is now released with coroutines 1.0

@JakeWharton

This comment has been minimized.

Copy link
Collaborator Author

JakeWharton commented Oct 29, 2018

Yes. It will still be a few weeks before this lands, but it's at least unblocked.

@matejdro

This comment has been minimized.

Copy link

matejdro commented Nov 14, 2018

From what I see, this MR completely bypasses call adapters when using suspend functions.

I'm currently using custom call adapters for centralising parsing of error body (and then throwing appropriate exceptions), smilarly to the official retrofit2 sample.

Any chance we get alternative to this, some kind of adapter that is injected between here?

@cbruegg

This comment has been minimized.

Copy link

cbruegg commented Nov 14, 2018

Would it be possible to implement this in such a way that stacktraces are properly augmented with actual call sites? There is WIP being done on this in kotlinx.coroutines: Kotlin/kotlinx.coroutines#792

This would apparently require using either of async, launch, coroutineScope, supervisorScope, withContext. See also this motivation.

@JakeWharton JakeWharton force-pushed the jakew/suspend/2018-09-11 branch 3 times, most recently from 5e60d18 to 0ec00e8 Nov 15, 2018

@cbruegg

This comment has been minimized.

Copy link

cbruegg commented Nov 15, 2018

@JakeWharton Should I file a feature request for this as well? #2886 (comment)

@bolein

This comment has been minimized.

Copy link

bolein commented Nov 15, 2018

@matejdro I throw out of an Interceptor for those purposes

@kitttn

This comment was marked as outdated.

Copy link

kitttn commented Dec 21, 2018

@JakeWharton
Maybe you'll have time to resolve this merge conflict one day?

suspend fun <T : Any> Call<T>.await(): T {
return suspendCancellableCoroutine { continuation ->
continuation.invokeOnCancellation {
cancel()

This comment has been minimized.

@swankjesse

swankjesse Feb 15, 2019

Member

super cool

This comment has been minimized.

@JakeWharton

JakeWharton Feb 15, 2019

Author Collaborator

Should probably test this. Can do in follow-up.

Show resolved Hide resolved retrofit/src/test/java/retrofit2/KotlinSuspendTest.kt Outdated

@JakeWharton JakeWharton force-pushed the jakew/suspend/2018-09-11 branch from 09c8f2c to b761518 Feb 15, 2019

@JakeWharton JakeWharton merged commit 3ee4950 into master Feb 15, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@JakeWharton JakeWharton deleted the jakew/suspend/2018-09-11 branch Feb 15, 2019

@TurKurT656

This comment has been minimized.

Copy link

TurKurT656 commented Feb 27, 2019

fantastic work. so do we need to add a call adapter for this to work? i didn't see any documentation about this.

@JakeWharton

This comment has been minimized.

Copy link
Collaborator Author

JakeWharton commented Feb 27, 2019

@Rody66

This comment has been minimized.

Copy link

Rody66 commented Feb 27, 2019

JakeWharton, many thanks for your great work! May you help us how to implement suspend in retrofit?

@droidluv

This comment has been minimized.

Copy link

droidluv commented Mar 17, 2019

I am not exactly sure what to do here, because cancelling a deferred job of retrofit doesn't cancel the network call, the api call seems to move to completion whether I cancelled it or not, is there a newer version of retrofit I can use which has this feature? Currently I am on retrofit 2.5.0.

@bradynpoulsen

This comment has been minimized.

Copy link

bradynpoulsen commented Mar 17, 2019

@droidluv this seems unrelated because 2.5.0 doesn't contain this PR. This PR is available on master (2.5.1-SNAPSHOT). Perhaps open new issues for any problems you experience

@droidluv

This comment has been minimized.

Copy link

droidluv commented Mar 18, 2019

@bradynpoulsen I apologise if I was unclear and vague, I was brought here from here, I realised the fix was on 2.5.1-SNAPSHOT
I have my gradle configured for
maven { url "https://oss.sonatype.org/content/repositories/snapshots" }
and has com.squareup.retrofit2:retrofit:2.5.1-SNAPSHOT as my dependency but it fails to resolve, and I am unsure on how to move forward

@Draptol

This comment has been minimized.

Copy link

Draptol commented Mar 21, 2019

I have a question about retrofit 2.5.1-SNAPSHOT and canceling request. I downloaded this version of retrofit from the repository
"https://oss.sonatype.org/content/repositories/snapshots" and my project was set up properly. Next, I tried to implement suspending method or await call like in this tutorial but after canceling parent job nothing happens. My request didn’t cancel and I got a response (I checked logs while the application was working). For tests, I use https://httpstat.us/ with delay after which I am getting the response from a server. So my question is how to implement canceling the request in the new retrofit version (which should support cancellation properly ). Could you give me any code snippet? In my scenario, I have a few requests which are running simultaneously using async and I need to cancel all of them by canceling the parent job.

@JakeWharton

This comment has been minimized.

Copy link
Collaborator Author

JakeWharton commented Mar 21, 2019

A cancelation test was added in #3029 which verifies that canceling the coroutine will propagate to the underlying Call.

@Draptol

This comment has been minimized.

Copy link

Draptol commented Mar 21, 2019

@JakeWharton I saw this test.
Maybe I am doing something wrong, but I have a problem with this cancellation.

Here you have my project's environment.

Gradle:

 // Retrofit 2.0
    implementation 'com.squareup.retrofit2:retrofit:2.5.1-SNAPSHOT'
    implementation 'com.squareup.retrofit2:converter-gson:2.5.1-SNAPSHOT'
    implementation 'com.squareup.okhttp3:logging-interceptor:3.11.0'
    implementation 'com.squareup.okhttp3:okhttp:3.12.0'

Routes:

interface Routes {
    @GET("/200?sleep=5000")
    suspend fun test(): ResponseBody

    @GET("/200?sleep=5000")
    fun test2(): Call<ResponseBody>
}

Server:

class NetworkService() {
    val routes: Routes

    init {
        val gson = GsonBuilder().setDateFormat(DateFormat.LONG).create()
        val retrofit = Retrofit.Builder()
            .addConverterFactory(GsonConverterFactory.create(gson))
            .client(
                createOkHttpClient(
                    connectionTimeout = 10 * 1000,
                    readTimeout = 10 * 1000,
                    writeTimeout = 10 * 1000
                )
            )
            .baseUrl("https://httpstat.us")
            .build()
        routes = retrofit.create(Routes::class.java)
    }

    private fun createOkHttpClient(
        connectionTimeout: Long,
        readTimeout: Long,
        writeTimeout: Long
    ): OkHttpClient {
        val debugInterceptor = HttpLoggingInterceptor().apply {
            level = HttpLoggingInterceptor.Level.BODY
        }

        return OkHttpClient.Builder()
            .addInterceptor(debugInterceptor)
            .connectTimeout(connectionTimeout, TimeUnit.MILLISECONDS)
            .readTimeout(readTimeout, TimeUnit.MILLISECONDS)
            .writeTimeout(writeTimeout, TimeUnit.MILLISECONDS)
            .build()
    }
}

Main Activity:

override fun onCreate(savedInstanceState: Bundle?) {
       super.onCreate(savedInstanceState)
       setContentView(R.layout.activity_main)

       val routes = NetworkService().routes
       runBlocking {
           val job = GlobalScope.launch {
               Log.d("NETWORK_DEBUG_TAG", "start request")
               routes.test()
           }
           Log.d("NETWORK_DEBUG_TAG", "before delay")
           delay(1500)
           Log.d("NETWORK_DEBUG_TAG", "cancel")
           job.cancel()
           Log.d("NETWORK_DEBUG_TAG", "job isCancelled = ${job.isCancelled}")
           job.join()
           Log.d("NETWORK_DEBUG_TAG", "after join")
       }
   }

After invoking above code I got these logs:

2019-03-21 11:46:50.665 21884-21884/com.test.testapp D/NETWORK_DEBUG_TAG: before delay
2019-03-21 11:46:50.666 21884-22069/com.test.testapp D/NETWORK_DEBUG_TAG: start request
2019-03-21 11:46:50.717 21884-22072/com.test.testapp D/OkHttp: --> GET https://httpstat.us/200?sleep=5000
2019-03-21 11:46:50.717 21884-22072/com.test.testapp D/OkHttp: --> END GET
2019-03-21 11:46:52.170 21884-21884/com.test.testapp D/NETWORK_DEBUG_TAG: cancel
2019-03-21 11:46:52.171 21884-21884/com.test.testapp D/NETWORK_DEBUG_TAG: job isCancelled = true
2019-03-21 11:46:57.102 21884-22072/com.test.testapp D/OkHttp: <-- 200 OK https://httpstat.us/200?sleep=5000 (6383ms)
2019-03-21 11:46:57.103 21884-22072/com.test.testapp D/OkHttp: Cache-Control: private
2019-03-21 11:46:57.103 21884-22072/com.test.testapp D/OkHttp: Server: Microsoft-IIS/10.0
2019-03-21 11:46:57.103 21884-22072/com.test.testapp D/OkHttp: X-AspNetMvc-Version: 5.1
2019-03-21 11:46:57.103 21884-22072/com.test.testapp D/OkHttp: Access-Control-Allow-Origin: *
2019-03-21 11:46:57.104 21884-22072/com.test.testapp D/OkHttp: X-AspNet-Version: 4.0.30319
2019-03-21 11:46:57.104 21884-22072/com.test.testapp D/OkHttp: X-Powered-By: ASP.NET
2019-03-21 11:46:57.104 21884-22072/com.test.testapp D/OkHttp: Set-Cookie: ARRAffinity=beec3692495883b8df6195e900c12f49514e054d865a22ad2951c84f51dbaf93;Path=/;HttpOnly;Domain=httpstat.us
2019-03-21 11:46:57.104 21884-22072/com.test.testapp D/OkHttp: Date: Thu, 21 Mar 2019 10:46:56 GMT
2019-03-21 11:46:57.106 21884-22072/com.test.testapp D/OkHttp: Content-Length: 0
2019-03-21 11:46:57.108 21884-22072/com.test.testapp D/OkHttp: <-- END HTTP (0-byte body)
2019-03-21 11:46:57.121 21884-21884/com.test.testapp D/NETWORK_DEBUG_TAG: after join

Test scenario:
Firstly, I make request to server which is to respond after 5 seconds (look at query param). After 1,5 second I cancel the job so I assume that the call would be Immediately cancelled. But it doesn't heppen. I look at logs and I see that request was completed although I had cancelled the job after 1,5 second.

So could you tell me what could be the problem?

@Jesus13

This comment has been minimized.

Copy link

Jesus13 commented Mar 21, 2019

Hey, I want to use the suspend function in my production ready project, but the suspend function is not yet in the stable branch. When do you plan to release 2.5.1? What issues are blocking the release?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.