-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Fix K/N EvenLoop.reschedule time conversion #4245
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
Conversation
| protected actual fun reschedule(now: Long, delayedTask: EventLoopImplBase.DelayedTask) { | ||
| DefaultExecutor.invokeOnTimeout(now, delayedTask, EmptyCoroutineContext) | ||
| val delayTimeMillis = delayNanosToMillis(nanoTime() - now) | ||
| DefaultExecutor.invokeOnTimeout(delayTimeMillis, delayedTask, EmptyCoroutineContext) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
|
||
| protected actual fun reschedule(now: Long, delayedTask: EventLoopImplBase.DelayedTask) { | ||
| DefaultExecutor.invokeOnTimeout(now, delayedTask, EmptyCoroutineContext) | ||
| val delayTimeMillis = delayNanosToMillis(nanoTime() - now) |
There was a problem hiding this comment.
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.DelayedTaskhas thenanoTimefield that tells us when the task has to run.nowshould already be fairly close tonanoTime(). Passingnowis a small optimization to avoid requerying nanoseconds all the time.
So, I think the correct version is
| val delayTimeMillis = delayNanosToMillis(nanoTime() - now) | |
| val delayTimeMillis = delayNanosToMillis(delayedTask.nanoTime - now) |
There was a problem hiding this comment.
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.
| protected actual fun reschedule(now: Long, delayedTask: EventLoopImplBase.DelayedTask) { | ||
| DefaultExecutor.invokeOnTimeout(now, delayedTask, EmptyCoroutineContext) | ||
| val delayTimeMillis = delayNanosToMillis(nanoTime() - now) | ||
| DefaultExecutor.invokeOnTimeout(delayTimeMillis, delayedTask, EmptyCoroutineContext) |
There was a problem hiding this comment.
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.
35a51b6 to
7eaa68b
Compare
|
Thanks! |
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.