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

Fix K/N EvenLoop.reschedule time conversion #4245

Merged
merged 2 commits into from
Oct 17, 2024

Conversation

qurbonzoda
Copy link
Contributor

A "point of time" value was wrongly passed to a function that expects a "duration of time". Additionally, the former was in nanos, while the later in milllis.

@@ -15,7 +15,8 @@ internal actual abstract class EventLoopImplPlatform : EventLoop() {
}

protected actual fun reschedule(now: Long, delayedTask: EventLoopImplBase.DelayedTask) {
DefaultExecutor.invokeOnTimeout(now, delayedTask, EmptyCoroutineContext)
val delayTimeMillis = delayNanosToMillis(nanoTime() - now)
DefaultExecutor.invokeOnTimeout(delayTimeMillis, delayedTask, EmptyCoroutineContext)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure what is the correct way to test this fix.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can think of this simple test:

    /** Tests that the delayed tasks scheduled on a closed `runBlocking` event loop get processed in reasonable time. */
    @Test
    fun testReschedulingDelayedTasks() {
        val job = runBlocking {
            val dispatcher = coroutineContext[ContinuationInterceptor]!!
            GlobalScope.launch(dispatcher) {
                delay(1.milliseconds)
            }
        }
        runBlocking {
            withTimeout(10.seconds) {
                job.join()
            }
        }
    }

This would miss the error in the opposite direction (like immediately executing the tasks instead), but testing that is trickier, so maybe let's leave it at that?

A good file for this test could be kotlinx-coroutines-core/concurrent/test/RunBlockingTest.kt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the test. It failed before the fix and succeeds now.

@@ -15,7 +15,8 @@ internal actual abstract class EventLoopImplPlatform : EventLoop() {
}

protected actual fun reschedule(now: Long, delayedTask: EventLoopImplBase.DelayedTask) {
DefaultExecutor.invokeOnTimeout(now, delayedTask, EmptyCoroutineContext)
val delayTimeMillis = delayNanosToMillis(nanoTime() - now)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think that's quite right.

  • EventLoopImplBase.DelayedTask has the nanoTime field that tells us when the task has to run.
  • now should already be fairly close to nanoTime(). Passing now is a small optimization to avoid requerying nanoseconds all the time.

So, I think the correct version is

Suggested change
val delayTimeMillis = delayNanosToMillis(nanoTime() - now)
val delayTimeMillis = delayNanosToMillis(delayedTask.nanoTime - now)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. I've applied the change.

@@ -15,7 +15,8 @@ internal actual abstract class EventLoopImplPlatform : EventLoop() {
}

protected actual fun reschedule(now: Long, delayedTask: EventLoopImplBase.DelayedTask) {
DefaultExecutor.invokeOnTimeout(now, delayedTask, EmptyCoroutineContext)
val delayTimeMillis = delayNanosToMillis(nanoTime() - now)
DefaultExecutor.invokeOnTimeout(delayTimeMillis, delayedTask, EmptyCoroutineContext)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I can think of this simple test:

    /** Tests that the delayed tasks scheduled on a closed `runBlocking` event loop get processed in reasonable time. */
    @Test
    fun testReschedulingDelayedTasks() {
        val job = runBlocking {
            val dispatcher = coroutineContext[ContinuationInterceptor]!!
            GlobalScope.launch(dispatcher) {
                delay(1.milliseconds)
            }
        }
        runBlocking {
            withTimeout(10.seconds) {
                job.join()
            }
        }
    }

This would miss the error in the opposite direction (like immediately executing the tasks instead), but testing that is trickier, so maybe let's leave it at that?

A good file for this test could be kotlinx-coroutines-core/concurrent/test/RunBlockingTest.kt.

A "point of time" value was wrongly passed to a function that expects a "duration of time". Additionally, the former was in nanos, while the later in milllis.
@qurbonzoda qurbonzoda force-pushed the qurbonzoda/fix-native-EventLoop-reschedule branch from 35a51b6 to 7eaa68b Compare October 17, 2024 09:17
@dkhalanskyjb dkhalanskyjb merged commit 1fcffbf into develop Oct 17, 2024
1 check passed
@dkhalanskyjb dkhalanskyjb deleted the qurbonzoda/fix-native-EventLoop-reschedule branch October 17, 2024 12:28
@dkhalanskyjb
Copy link
Collaborator

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants