asyncevents: fix missing GC root and race #44956
Conversation
The event might have triggered on another thread before we observed it here, or it might have gotten finalized before it got triggered. Either outcome can result in a lost event. (I observed the later situation occurring locally during the Dates test once).
vtjnash
added
backport 1.7
backport 1.8
| async = AsyncCondition() | ||
| t = @task while _trywait(async) | ||
| cb(async) | ||
| isopen(async) || return | ||
| t = @task begin | ||
| unpreserve_handle(async) |
There was a problem hiding this comment.
Isn't async reachable through t.code? Why would it be finalized in the old code?
There was a problem hiding this comment.
t is only reachable through async
| @@ -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 | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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()
endand 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.
There was a problem hiding this comment.
Establishing happens-before seems like a good idea. We don't do that currently, so it is not relevant to this PR however.
The event might have triggered on another thread before we observed it here, or it might have gotten finalized before it got triggered. Either outcome can result in a lost event. (I observed the later situation occurring locally during the Dates test once). (cherry picked from commit 8cc00ff)
The event might have triggered on another thread before we observed it here, or it might have gotten finalized before it got triggered. Either outcome can result in a lost event. (I observed the later situation occurring locally during the Dates test once). (cherry picked from commit 8cc00ff)
KristofferC
removed
backport 1.8
The event might have triggered on another thread before we observed it
here, or it might have gotten finalized before it got triggered. Either
outcome can result in a lost event. (I observed the later situation
occurring locally during the Dates test once).