Skip to content

Ensure awaitFrame() only awaits a single frame when used from the main looper #3437

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

Merged
merged 4 commits into from
Sep 16, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 12 additions & 7 deletions ui/kotlinx-coroutines-android/src/HandlerDispatcher.kt
Original file line number Diff line number Diff line change
Expand Up @@ -185,22 +185,27 @@ private var choreographer: Choreographer? = null
public suspend fun awaitFrame(): Long {
// fast path when choreographer is already known
val choreographer = choreographer
if (choreographer != null) {
return suspendCancellableCoroutine { cont ->
return if (choreographer != null) {
suspendCancellableCoroutine { cont ->
postFrameCallback(choreographer, cont)
}
} else {
awaitFrameSlowPath()
}
// post into looper thread to figure it out
return suspendCancellableCoroutine { cont ->
Dispatchers.Main.dispatch(EmptyCoroutineContext, Runnable {
}

private suspend fun awaitFrameSlowPath(): Long = suspendCancellableCoroutine { cont ->
if (Looper.myLooper() === Looper.getMainLooper()) { // Check if we are already in the main looper thread
updateChoreographerAndPostFrameCallback(cont)
Copy link
Collaborator

@dkhalanskyjb dkhalanskyjb Sep 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw you mention Dispatchers.Main.immediate not being good enough for this and something about intending to fix it. Could you elaborate?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I was hoping that adding Dispatchers.main.immediate would solve the problem, but it won't due to how we do structure our code: CancellableContinuation already incapsulates the dispatcher (for this case -- Dispatchers.Main) and we cannot "just" resume it in place.

Neither can we do Dispatchers.Main.immediate.resumeUndispatched(cont) due to how it is implemented -- that's a grey area in "how should 'resumeUndispatched' behave if extension receiver and actual dispatcher do not match". And they do not match because Dispatchers.Main.immediate and Dispatchers.Main are different dispatchers, non-transparent to each other

} else { // post into looper thread to figure it out
Dispatchers.Main.dispatch(cont.context, Runnable {
updateChoreographerAndPostFrameCallback(cont)
})
}
}

private fun updateChoreographerAndPostFrameCallback(cont: CancellableContinuation<Long>) {
val choreographer = choreographer ?:
Choreographer.getInstance()!!.also { choreographer = it }
val choreographer = choreographer ?: Choreographer.getInstance()!!.also { choreographer = it }
postFrameCallback(choreographer, cont)
}

Expand Down
8 changes: 3 additions & 5 deletions ui/kotlinx-coroutines-android/test/HandlerDispatcherTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -109,16 +109,14 @@ class HandlerDispatcherTest : TestBase() {
launch(Dispatchers.Main, start = CoroutineStart.UNDISPATCHED) {
expect(1)
awaitFrame()
expect(5)
expect(3)
}
expect(2)
// Run choreographer detection
mainLooper.runOneTask()
expect(3)
mainLooper.scheduler.advanceBy(50, TimeUnit.MILLISECONDS)
expect(4)
mainLooper.scheduler.advanceBy(51, TimeUnit.MILLISECONDS)
finish(6)
mainLooper.scheduler.advanceBy(50, TimeUnit.MILLISECONDS)
finish(5)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then it's finish(4), as the action after expect(4) also does nothing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

}

private fun CoroutineScope.doTestAwaitWithDetectedChoreographer() {
Expand Down
37 changes: 0 additions & 37 deletions ui/kotlinx-coroutines-javafx/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -21,40 +21,3 @@ javafx {
modules = listOf("javafx.controls")
configuration = "javafx"
}

val JDK_18: String? by lazy {
System.getenv("JDK_18")
}

val checkJdk8 by tasks.registering {
// only fail w/o JDK_18 when actually trying to test, not during project setup phase
doLast {
if (JDK_18 == null) {
throw GradleException(
"""
JDK_18 environment variable is not defined.
Can't run JDK 8 compatibility tests.
Please ensure JDK 8 is installed and that JDK_18 points to it.
""".trimIndent()
)
}
}
}

val jdk8Test by tasks.registering(Test::class) {
// Run these tests only during nightly stress test
onlyIf { project.properties["stressTest"] != null }

val test = tasks.test.get()

classpath = test.classpath
testClassesDirs = test.testClassesDirs
executable = "$JDK_18/bin/java"

dependsOn("compileTestKotlin")
dependsOn(checkJdk8)
}

tasks.build {
dependsOn(jdk8Test)
}
11 changes: 6 additions & 5 deletions ui/kotlinx-coroutines-javafx/src/JavaFxDispatcher.kt
Original file line number Diff line number Diff line change
Expand Up @@ -34,22 +34,22 @@ public sealed class JavaFxDispatcher : MainCoroutineDispatcher(), Delay {

/** @suppress */
override fun scheduleResumeAfterDelay(timeMillis: Long, continuation: CancellableContinuation<Unit>) {
val timeline = schedule(timeMillis, TimeUnit.MILLISECONDS) {
val timeline = schedule(timeMillis) {
with(continuation) { resumeUndispatched(Unit) }
}
continuation.invokeOnCancellation { timeline.stop() }
}

/** @suppress */
override fun invokeOnTimeout(timeMillis: Long, block: Runnable, context: CoroutineContext): DisposableHandle {
val timeline = schedule(timeMillis, TimeUnit.MILLISECONDS) {
val timeline = schedule(timeMillis) {
block.run()
}
return DisposableHandle { timeline.stop() }
}

private fun schedule(time: Long, unit: TimeUnit, handler: EventHandler<ActionEvent>): Timeline =
Timeline(KeyFrame(Duration.millis(unit.toMillis(time).toDouble()), handler)).apply { play() }
private fun schedule(timeMillis: Long, handler: EventHandler<ActionEvent>): Timeline =
Timeline(KeyFrame(Duration.millis(timeMillis.toDouble()), handler)).apply { play() }
}

internal class JavaFxDispatcherFactory : MainDispatcherFactory {
Expand Down Expand Up @@ -97,7 +97,7 @@ public suspend fun awaitPulse(): Long = suspendCancellableCoroutine { cont ->
}

private class PulseTimer : AnimationTimer() {
val next = CopyOnWriteArrayList<CancellableContinuation<Long>>()
private val next = CopyOnWriteArrayList<CancellableContinuation<Long>>()

override fun handle(now: Long) {
val cur = next.toTypedArray()
Expand All @@ -116,6 +116,7 @@ internal fun initPlatform(): Boolean = PlatformInitializer.success

// Lazily try to initialize JavaFx platform just once
private object PlatformInitializer {
@JvmField
val success = run {
/*
* Try to instantiate JavaFx platform in a way which works
Expand Down
17 changes: 6 additions & 11 deletions ui/kotlinx-coroutines-swing/src/SwingDispatcher.kt
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ package kotlinx.coroutines.swing
import kotlinx.coroutines.*
import kotlinx.coroutines.internal.*
import java.awt.event.*
import java.util.concurrent.*
import javax.swing.*
import kotlin.coroutines.*

Expand All @@ -29,26 +28,22 @@ public sealed class SwingDispatcher : MainCoroutineDispatcher(), Delay {

/** @suppress */
override fun scheduleResumeAfterDelay(timeMillis: Long, continuation: CancellableContinuation<Unit>) {
val timer = schedule(timeMillis, TimeUnit.MILLISECONDS, ActionListener {
val timer = schedule(timeMillis) {
with(continuation) { resumeUndispatched(Unit) }
})
}
continuation.invokeOnCancellation { timer.stop() }
}

/** @suppress */
override fun invokeOnTimeout(timeMillis: Long, block: Runnable, context: CoroutineContext): DisposableHandle {
val timer = schedule(timeMillis, TimeUnit.MILLISECONDS, ActionListener {
val timer = schedule(timeMillis) {
block.run()
})
return object : DisposableHandle {
override fun dispose() {
timer.stop()
}
}
return DisposableHandle { timer.stop() }
}

private fun schedule(time: Long, unit: TimeUnit, action: ActionListener): Timer =
Timer(unit.toMillis(time).coerceAtMost(Int.MAX_VALUE.toLong()).toInt(), action).apply {
private fun schedule(timeMillis: Long, action: ActionListener): Timer =
Timer(timeMillis.coerceAtMost(Int.MAX_VALUE.toLong()).toInt(), action).apply {
isRepeats = false
start()
}
Expand Down