Skip to content

Commit

Permalink
Avoid synchronous checked exceptions in 'suspend fun's
Browse files Browse the repository at this point in the history
Because of a race condition caused by thread preemption, this can sometimes happen. Explicitly yield to ensure the exception is delivered asynchronously to the continuation.

(cherry picked from commit 60c92e3)
  • Loading branch information
JakeWharton committed Jul 31, 2019
1 parent 27ee486 commit a697178
Show file tree
Hide file tree
Showing 3 changed files with 44 additions and 3 deletions.
19 changes: 16 additions & 3 deletions retrofit/src/main/java/retrofit2/HttpServiceMethod.java
Original file line number Diff line number Diff line change
Expand Up @@ -186,9 +186,22 @@ static final class SuspendForBody<ResponseT> extends HttpServiceMethod<ResponseT

//noinspection unchecked Checked by reflection inside RequestFactory.
Continuation<ResponseT> continuation = (Continuation<ResponseT>) args[args.length - 1];
return isNullable
? KotlinExtensions.awaitNullable(call, continuation)
: KotlinExtensions.await(call, continuation);

// Calls to OkHttp Call.enqueue() like those inside await and awaitNullable can sometimes
// invoke the supplied callback with an exception before the invoking stack frame can return.
// Coroutines will intercept the subsequent invocation of the Continuation and throw the
// exception synchronously. A Java Proxy cannot throw checked exceptions without them being
// declared on the interface method. To avoid the synchronous checked exception being wrapped
// in an UndeclaredThrowableException, it is intercepted and supplied to a helper which will
// force suspension to occur so that it can be instead delivered to the continuation to
// bypass this restriction.
try {
return isNullable
? KotlinExtensions.awaitNullable(call, continuation)
: KotlinExtensions.await(call, continuation);
} catch (Exception e) {
return KotlinExtensions.yieldAndThrow(e, continuation);
}
}
}
}
6 changes: 6 additions & 0 deletions retrofit/src/main/java/retrofit2/KotlinExtensions.kt
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
package retrofit2

import kotlinx.coroutines.suspendCancellableCoroutine
import kotlinx.coroutines.yield
import kotlin.coroutines.resume
import kotlin.coroutines.resumeWithException

Expand Down Expand Up @@ -95,3 +96,8 @@ suspend fun <T : Any> Call<T>.awaitResponse(): Response<T> {
})
}
}

internal suspend fun Exception.yieldAndThrow(): Nothing {
yield()
throw this
}
22 changes: 22 additions & 0 deletions retrofit/src/test/java/retrofit2/KotlinSuspendTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -273,4 +273,26 @@ class KotlinSuspendTest {
val body = runBlocking { example.body() }
assertThat(body).isEqualTo("HiHiHiHiHi")
}

@Test fun checkedExceptionsAreNotSynchronouslyThrown() = runBlocking {
val retrofit = Retrofit.Builder()
.baseUrl("https://unresolved-host.com/")
.addConverterFactory(ToStringConverterFactory())
.build()
val example = retrofit.create(Service::class.java)

server.shutdown()

// The problematic behavior of the UnknownHostException being synchronously thrown is
// probabilistic based on thread preemption. Running a thousand times will almost always
// trigger it, so we run an order of magnitude more to be safe.
repeat(10000) {
try {
example.body()
fail()
} catch (_: IOException) {
// We expect IOException, the bad behavior will wrap this in UndeclaredThrowableException.
}
}
}
}

0 comments on commit a697178

Please sign in to comment.