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

DefaultExecutor can never exit on IDLE #856

Closed
cy6erGn0m opened this issue Nov 22, 2018 · 1 comment
Closed

DefaultExecutor can never exit on IDLE #856

cy6erGn0m opened this issue Nov 22, 2018 · 1 comment
Assignees
Labels

Comments

@cy6erGn0m
Copy link
Contributor

cy6erGn0m commented Nov 22, 2018

See DefaultExecutor.run. According to it's source, if there are no tasks then processNextEvent() returns Long.MAX_VALUE. Then we check shutdownNanos and if not yet initialized then we check it again for the same condition (that is always true) and compute parkNanos time. After parking for KEEP_ALIVE_NANOS we respin here again but we are falling into the second case where parkNanos = parkNanos.coerceAtMost(KEEP_ALIVE_NANOS) is executed and the same after one more respin so we will never exit but spin every second instead.

Also it looks like we need to reset shutdownNanos if there was a task scheduled but we don't do it.

@mirjalal
Copy link

Any fix for this?

dkhalanskyjb added a commit that referenced this issue Mar 23, 2020
qwwdfsad pushed a commit that referenced this issue Apr 24, 2020
* Fix DefaultExecutor not being able to exit.
* Also adds the performance optimization. See the discussion on the PR.
* Add a stress test for the DefaultExecutor worker shutting down.
* Make `testDelayChannelBackpressure2` not fail:

This test could in theory already fail on the second
`checkNotEmpty`: after the first `checkNotEmpty` has passed,
first, the ticker channel awakens to produce a new element, and
then the main thread awakens to check if it's there. However, if
the ticker channel is sufficiently slowed down, it may not produce
the element in time for the main thread to find it.

After introducing the change that allows the worker thread in
`DefaultExecutor` to shut down, the initial delay of 2500 ms is
sufficient for the worker to shut down (which, by default, happens
after 1000 ms of inactivity), and then the aforementioned race
condition worsens: additional time is required to launch a new
worker thread and it's much easier to miss the deadline.

Now, the delays are much shorter, meaning that the worker thread is
never inactive long enough to shut down.

Fixes #856
recheej pushed a commit to recheej/kotlinx.coroutines that referenced this issue Dec 28, 2020
* Fix DefaultExecutor not being able to exit.
* Also adds the performance optimization. See the discussion on the PR.
* Add a stress test for the DefaultExecutor worker shutting down.
* Make `testDelayChannelBackpressure2` not fail:

This test could in theory already fail on the second
`checkNotEmpty`: after the first `checkNotEmpty` has passed,
first, the ticker channel awakens to produce a new element, and
then the main thread awakens to check if it's there. However, if
the ticker channel is sufficiently slowed down, it may not produce
the element in time for the main thread to find it.

After introducing the change that allows the worker thread in
`DefaultExecutor` to shut down, the initial delay of 2500 ms is
sufficient for the worker to shut down (which, by default, happens
after 1000 ms of inactivity), and then the aforementioned race
condition worsens: additional time is required to launch a new
worker thread and it's much easier to miss the deadline.

Now, the delays are much shorter, meaning that the worker thread is
never inactive long enough to shut down.

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

No branches or pull requests

4 participants