-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Add timeout parameter to wait(::Condition)
#56974
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
# This file is a part of Julia. License is MIT: https://julialang.org/license | ||
|
||
using Random | ||
using Base.Threads | ||
using Base: Experimental | ||
using Base: n_avail | ||
|
||
|
@@ -39,6 +40,22 @@ end | |
@test fetch(t) == "finished" | ||
end | ||
|
||
@testset "timed wait on Condition" begin | ||
a = Condition() | ||
@test_throws ArgumentError wait(a; timeout=0.0005) | ||
@test_throws TimeoutError wait(a; timeout=0.1) | ||
@spawn begin | ||
sleep(0.01) | ||
notify(a) | ||
end | ||
@test try | ||
wait(a; timeout=2) | ||
true | ||
catch | ||
false | ||
end | ||
Comment on lines
+47
to
+56
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a racy test, right? If we're running with 2 threads, there's a chance the spawned task could notify and you'd miss it. I think you could make it not racy by using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah yeah, that |
||
end | ||
|
||
@testset "various constructors" begin | ||
c = Channel() | ||
@test eltype(c) == Any | ||
|
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.
This appears to introduce a data race though, so we cannot merge this
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.
How's that? We're locking the condition variable here.
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.
This Timer runs concurrently with the return from
wait
, so by the time this code runs, you might have just corrupted some arbitrary subsequent wait on the same condition or by the time you schedule the TimeoutError, it could blow up some completely unrelated waitThere 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.
Ah, okay. There's an ABA problem. Let me see if I can find a solution for that.
But the waiting task is only scheduled with a
TimeoutError
if it was in this condition's wait queue, so I'm not sure I understand your "or" case here -- the only subsequentwait
that could get blown up is await
on the same condition, which is the same ABA problem?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.
It could been in the waitq, then removed before you got around to scheduling it, or vice versa with some other thread scheduling before it got around to removing it from the queue. Those codes are running on other threads, so it could be concurrent. There is potentially no guarantee that you can safely mutate this data-structure concurrently on two threads (#55542)
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.
Pushed a fix for the ABA problem that relies on happens-before -- if the waiter was scheduled, it sets
waiter_left
before returning. It can only re-enter the condition's wait queue by another call towait
, for which it must acquire the lock.We acquire the condition's lock before checking
waiter_left
and for the task's presence in the wait queue. If the task is present, it can only be because it has not been scheduled, because if it was scheduled, it would have setwaiter_left
before re-entering the wait queue.I think the combination of the lock and the atomic assure there is no ABA problem.
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.
We acquire the lock, confirm that the waiter did not leave and remove it from the wait queue before scheduling it. If it was not in the wait queue, we do not schedule it and this decision is made while holding the lock.
If the task is scheduled by
notify
, then it is removed from the condition's wait queue before it is scheduled, which is done while holding the condition's lock. If it is not in the wait queue, then we do not schedule it.