improve yieldto error handling #21458
Conversation
vtjnash
force-pushed
the
jn/yieldto-error
branch
from
5feb496
to
1bca23b
Compare
base/event.jl
Outdated
| @@ -150,19 +150,64 @@ tasks. | |||
| yield() = (enq_work(current_task()); wait()) | |||
|
|
|||
| """ | |||
| yield(t::Task, arg = nothing) | |||
|
|
|||
| A fast, unfair-scheduling version of "schedule(t, arg); yield()" which | |||
There was a problem hiding this comment.
shouldn't this be code highlighted rather than " quoted?
There was a problem hiding this comment.
Should it? I feel like I've been told I'm wrong whichever way I choose.
There was a problem hiding this comment.
" quotes are for strings in code, not so much code in docstrings
vtjnash
force-pushed
the
jn/yieldto-error
branch
from
fbfc03c
to
1f638c8
Compare
base/event.jl
Outdated
| yield(t::Task, arg = nothing) | ||
|
|
||
| A fast, unfair-scheduling version of `schedule(t, arg); yield()` which | ||
| immediately yields to "t" before calling the scheduler. |
There was a problem hiding this comment.
t should also use backticks
previously we couldn't reliably tell the difference between a failure to context switch and an explicitly scheduled error in the former case, we often need to undo some prior action by moving the code for the later into Julia, we can reliably run the necessary undo action when required fix #21442
vtjnash
force-pushed
the
jn/yieldto-error
branch
from
1f638c8
to
f9b959f
Compare
| return try_yieldto(Void, t) | ||
| end | ||
|
|
||
| function try_yieldto(undo::F, t::Task) where F |
There was a problem hiding this comment.
it ensures that this gets a specialized call site
There was a problem hiding this comment.
maybe worth noting that with a comment so it doesn't accidentally get removed
There was a problem hiding this comment.
How would an accident like that happen? Just think of this as the opposite of ::ANY (which we also don't need comments on).
There was a problem hiding this comment.
would it cause a test failure if removed? it looks unnecessary, is it documented that this forces specialization and when that's necessary?
There was a problem hiding this comment.
As I said, it's like ::ANY – it will never cause a tests failure, it's generally unnecessary, and nobody would probably notice
There was a problem hiding this comment.
and, like ::ANY it could really use comments when it's doing something important
There was a problem hiding this comment.
I think @tkelman is right here. There are a lot of type signatures that end up having unnecessary type parameters for various reasons, which we tend to reflexively remove. ANY only gets used for a reason, which we don't reflexively use. So unless you comment this or test it somehow, you should expect that this will get deleted at some point.
base/stream.jl
Outdated
| @@ -794,7 +794,7 @@ uv_write(s::LibuvStream, p::Vector{UInt8}) = uv_write(s, pointer(p), UInt(sizeof | |||
| function uv_write(s::LibuvStream, p::Ptr{UInt8}, n::UInt) | |||
| check_open(s) | |||
| uvw = Libc.malloc(_sizeof_uv_write) | |||
| uv_req_set_data(uvw,C_NULL) | |||
| uv_req_set_data(uvw, C_NULL) # in-case we get interrupted before arriving at the wait call | |||
vtjnash
force-pushed
the
jn/yieldto-error
branch
from
f9b959f
to
011810a
Compare
|
This broke multiple packages, anything that was relying on the deprecation of produce/consume. Ex: https://github.com/JuliaCI/pkg.julialang.org/blob/64fea61d49dc90d55b5456b77afdbbc17e132a6c/logs/Erdos_0.6.log |
| # fast version of schedule(t,v);wait() | ||
| function schedule_and_wait(t::Task, v=nothing) | ||
| # fast version of `schedule(t, arg); wait()` | ||
| function schedule_and_wait(t::Task, arg=nothing) |
There was a problem hiding this comment.
v=nothing or change v to arg in the body.
And a test for schedule_and_wait
ensure that Windows libc stdio is also redirected by redirect_stdio, since jl_safe_printf is using the msvcrt-compatibility library rather than directly using the Win32 API fix issue mentioned in #21458
|
ref andrewcooke/ParserCombinator.jl#24 (which breaks GraphIO and LightGraphs v0.8). (Same issue as Erdos.) |
* Improve code-coverage testing of event scheduler so that we would have caught this breakage in CI * Ensure that Windows libc stdio is also redirected by redirect_stdio, since jl_safe_printf is using the msvcrt-compatibility library rather than directly using the Win32 API * Stop leaking a handle to the current process to child processes
previously we couldn't reliably tell the difference between a failure to context switch
and an explicitly scheduled error
in the former case, we often need to undo some prior action
by moving the code for the later into Julia,
we can reliably run the necessary undo action when required
fix #21442