-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Implement isopen() for Base.Event #54503
base: master
Are you sure you want to change the base?
Conversation
This is useful to know if the event is signaled or not without checking the `set` field, and it matches the API of `AsyncCondition` and `Timer`.
!!! compat "Julia 1.12" | ||
`isopen(::Event)` requires at least Julia 1.12. | ||
""" |
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 compat notice should be part of an Event
-specific docstring on the isopen
definition below. The generic isopen
has a docstring that isn't really applicable to Event
, since it talks about objects that produce events.
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.
Right, on second thoughts this is actually a confusing implementation since Event
can produce multiple events with autoreset=True
. Changed the implementation in 70f719e to match isopen()
better.
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 has time-traveling behavior that makes it mostly not usable due to the lack of use of atomics, that I don't really think is desirable. But maybe just making it seq_cst is enough to make it useable as an API? It must document the threading synchronization guarantees whatever they are as well
tasks will no longer block when waiting for it, until `reset` is called. | ||
tasks will no longer block when waiting for it, until [`reset(::Event)`](@ref) | ||
is called. Use [`isopen`](@ref) to check if it can still be signaled without | ||
having to call [`reset(::Event)`](@ref). |
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 don't know what to is means, as an event can be signaled again even if not reset, and the wording seems to imply some sort of seq_cst behavior, which isn't something that multiple operations (check and reset) is valid terminology to have independently. The behavior when autoreset also seems contradictory to the statements here, as it talks about isopen being a property of writing (which is commonly true), but then associates it with reset, which is a reader operation and I don't think that synchronizes with writes, broadly speaking
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.
Maybe signaled is the wrong word, what I mean is that isopen
can be used to check if the event will ever change from a un-set state to a set state again without having to explicitly reset
it. Or put another way, if calling notify
on the Event
will have any effect.
The behavior when autoreset also seems contradictory to the statements here, as it talks about isopen being a property of writing (which is commonly true), but then associates it with reset, which is a reader operation and I don't think that synchronizes with writes, broadly speaking
I'm not sure I quite understand what you mean, but I think the confusion might be because of the phrase "be signaled" coming across as 'you can signal this' instead of 'the state can change to signaled'. An alternative wording could be something like "Use isopen
to check if it will ever block again while waiting without having to call reset
"?
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.
"will have" sounds like a guarantee of some sort though, but it comes with so many asterisks (that it is not autoreset, that there is only one producer calling notify, and that the consumer does not call reset), I find the combination seems confusing how it would be used in practice
Ah, should I be using |
yes, just |
This is useful to know if the event is signaled or not without checking the, and it matches the API ofset
fieldAsyncCondition
andTimer
.