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.

Sign up for GitHub

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

Jump to bottom

asyncevents: fix missing GC root and race #44956

Merged
merged 1 commit into from
Apr 13, 2022
Merged

asyncevents: fix missing GC root and race #44956

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 42 additions & 25 deletions base/asyncevent.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,22 @@ the async condition object itself.
"""
function AsyncCondition(cb::Function)
async = AsyncCondition()
t = @task while _trywait(async)
cb(async)
isopen(async) || return
t = @task begin
unpreserve_handle(async)
Comment on lines 47 to +49
Copy link
Member

Choose a reason for hiding this comment

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

Isn't async reachable through t.code? Why would it be finalized in the old code?

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

t is only reachable through async

Copy link
Member

Choose a reason for hiding this comment

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

ahhhh

while _trywait(async)
cb(async)
isopen(async) || return
end
end
# here we are mimicking parts of _trywait, in coordination with task `t`
preserve_handle(async)
@lock async.cond begin
if async.set
schedule(t)
else
_wait2(async.cond, t)
end
end
lock(async.cond)
_wait2(async.cond, t)
unlock(async.cond)
return async
end

Expand Down Expand Up @@ -115,6 +124,7 @@ function _trywait(t::Union{Timer, AsyncCondition})
# full barrier now for AsyncCondition
t isa Timer || Core.Intrinsics.atomic_fence(:acquire_release)
else
t.isopen || return false
Copy link
Member

@tkf tkf Apr 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Since this is outside a lock, don't we need to acquire load/release store t.isopen for AsyncCondition so that close(async) happens-before wait(async) throws? Not sure if that's needed though.

Copy link
Member

@tkf tkf Apr 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think it's conceivable that someone writes

async = AsyncCondition()

t1 = @spawn begin  # code in external library
    f1()
    ccall(:uv_async_send, Cvoid, (Ptr{Cvoid},), async)
end

t2 = @spawn begin
    try
        wait(async)
    catch err
        err isa EOFError || rethrow()
    end
    close(async)
    f2()
end

t3 = @spawn begin
    try
        wait(async)
    catch err
        err isa EOFError || rethrow()
    end
    close(async)
    f3()
end

and expect f1 to happen-before f2 and f3. There are better ways to do level-triggering based on AsyncCondition but it does not look super crazy to me.

But I'm also OK with documenting that wait does not establish any ordering if it throws.

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

Establishing happens-before seems like a good idea. We don't do that currently, so it is not relevant to this PR however.

t.handle == C_NULL && return false
iolock_begin()
set = t.set
Expand All @@ -123,14 +133,12 @@ function _trywait(t::Union{Timer, AsyncCondition})
lock(t.cond)
try
set = t.set
if !set
if t.handle != C_NULL
iolock_end()
set = wait(t.cond)
unlock(t.cond)
iolock_begin()
lock(t.cond)
end
if !set && t.isopen && t.handle != C_NULL
iolock_end()
set = wait(t.cond)
unlock(t.cond)
iolock_begin()
lock(t.cond)
end
finally
unlock(t.cond)
Expand Down Expand Up @@ -266,19 +274,28 @@ julia> begin
"""
function Timer(cb::Function, timeout::Real; interval::Real=0.0)
timer = Timer(timeout, interval=interval)
t = @task while _trywait(timer)
try
cb(timer)
catch err
write(stderr, "Error in Timer:\n")
showerror(stderr, err, catch_backtrace())
return
t = @task begin
unpreserve_handle(timer)
while _trywait(timer)
try
cb(timer)
catch err
write(stderr, "Error in Timer:\n")
showerror(stderr, err, catch_backtrace())
return
end
isopen(timer) || return
end
end
# here we are mimicking parts of _trywait, in coordination with task `t`
preserve_handle(timer)
@lock timer.cond begin
if timer.set
schedule(t)
else
_wait2(timer.cond, t)
end
isopen(timer) || return
end
lock(timer.cond)
_wait2(timer.cond, t)
unlock(timer.cond)
return timer
end

Expand Down