-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
Scheduler: Use a "scheduler" task for thread sleep #57544
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
Conversation
b345e87
to
4c08ecb
Compare
4c08ecb
to
aa69cb7
Compare
This is currently blocked on what seems to be a bug in |
…simage (#57656) This is quite tricky to test unfortunately, but #57544 caught this and this fixes that --------- Co-authored-by: Jameson Nash <[email protected]>
f280243
to
684637a
Compare
…simage (#57656) This is quite tricky to test unfortunately, but #57544 caught this and this fixes that --------- Co-authored-by: Jameson Nash <[email protected]> (cherry picked from commit bf01638)
Unblocked... thanks @gbaraldi! Now, some |
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 think those tests are specifically testing race conditions in the code, so you might need to adjust them slightly to account for the change in process_events
ordering and count
Is there any chance of getting this backported to 1.12? It's quite tricky to debug and would be nice if it was fixed in the next release. |
684637a
to
3bcde1b
Compare
This small group of tests is written with assumptions about when and how the libuv event loop is run. As this PR changes this behavior, the tests needed adjusting.
Previously, this test depended on scheduler behavior, which is slightly changed in this PR. Changed the test to connect to a non-routable IP address so that it no longer depends on task ordering.
Thanks @vtjnash for the idea on how to fix the Sockets test. |
The |
# We may have already switched tasks (via the scheduler task), so | ||
# only switch if we haven't. | ||
if !have_result | ||
@assert task isa Task |
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.
Any specific reason this doesn't just typeassert? Just curious.
@assert task isa Task | |
task = task::Task |
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.
No specific reason. Seems like it should be a typeassert, actually.
A Julia thread runs Julia's scheduler in the context of the switching task. If no task is found to switch to, the thread will sleep while holding onto the (possibly completed) task, preventing the task from being garbage collected. This recent [Discourse post](https://discourse.julialang.org/t/weird-behaviour-of-gc-with-multithreaded-array-access/125433) illustrates precisely this problem. A solution to this would be for an idle Julia thread to switch to a "scheduler" task, thereby freeing the old task. This PR uses `OncePerThread` to create a "scheduler" task (that does nothing but run `wait()` in a loop) and switches to that task when the thread finds itself idle. Other approaches considered and discarded in favor of this one: #57465 and #57543. (cherry picked from commit 0d4d6d9)
A typeassert seems like better style. Given that `typeassert` is a builtin, why not put it to use. See: * JuliaLang#57544 (comment)
A typeassert seems like better style. Given that `typeassert` is a builtin, why not put it to use. See: * JuliaLang#57544 (comment)
A typeassert seems like better style. Given that `typeassert` is a builtin, why not put it to use. See: * JuliaLang#57544 (comment)
This change interacts badly with ^C. Many interrupt exception will now get thrown to the scheduler task, which then dies and takes down the ability to ever schedule again in the future. |
Ugh.
|
I think we need to revert this for 1.12 and then work on implementing a proper behavior for 1.13 (#52291). |
We could also simply never direct the interrupt at scheduler tasks? |
That needs an extra feature to find the correct task to send it to then. This is too much for 1.12 at this point - we should reset to the old behavior for the release and we can work on something better for 1.13. |
This reverts commit 640c4e1.
A Julia thread runs Julia's scheduler in the context of the switching task. If no task is found to switch to, the thread will sleep while holding onto the (possibly completed) task, preventing the task from being garbage collected. This recent Discourse post illustrates precisely this problem.
A solution to this would be for an idle Julia thread to switch to a "scheduler" task, thereby freeing the old task.
This PR uses
OncePerThread
to create a "scheduler" task (that does nothing but runwait()
in a loop) and switches to that task when the thread finds itself idle.Other approaches considered and discarded in favor of this one: #57465 and #57543.