-
-
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
small enhancements to Task #13963
small enhancements to Task #13963
Changes from all commits
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 |
---|---|---|
|
@@ -114,7 +114,6 @@ | |
|
||
# type Task | ||
# parent::Task | ||
# last::Task | ||
# storage::Any | ||
# consumers | ||
# started::Bool | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1010,7 +1010,6 @@ export | |
Condition, | ||
consume, | ||
current_task, | ||
istaskstarted, | ||
istaskdone, | ||
lock, | ||
notify, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -348,13 +348,13 @@ end | |
|
||
function serialize(s::SerializationState, t::Task) | ||
serialize_cycle(s, t) && return | ||
if istaskstarted(t) && !istaskdone(t) | ||
if !istaskdone(t) | ||
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. With the old version of this code, it was possible to make a task and then serialize it right away (a slightly strange thing to do, but it could fall out of reasonable code quasi-accidentally). Is that no longer possible? 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. Sure, I just don't see how it could safely fall out of reasonable code. This condition introduces behavior whereby you can create copies of a Task, but only until it starts running. However, since the next few lines call serialize (and thus yield), the Task could start running while this function is going leading to inconsistent serialization results. (I realize the same could be said for all of the serialize methods, since we don't buffer / deepcopy the whole tree first, but perhaps such is life) I think there are ways of avoiding this, depending on the desired behavior. A stable result is guaranteed once the task is finished, however, making that state the only state that is definitely safe to serialize. |
||
error("cannot serialize a running Task") | ||
end | ||
writetag(s.io, TASK_TAG) | ||
serialize(s, t.code) | ||
serialize(s, t.storage) | ||
serialize(s, t.state == :queued || t.state == :waiting ? (:runnable) : t.state) | ||
serialize(s, t.state == :queued || t.state == :runnable ? (:runnable) : t.state) | ||
serialize(s, t.result) | ||
serialize(s, t.exception) | ||
end | ||
|
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 removes an exported function without a deprecation. Is there now no way to tell that a task has started?
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.
C should be able to still compute this if needed
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.
Then we should have this function call such C code, and then put a proper deprecation warning on it if we're actually going to remove it. I don't see why though. Is there a good reason to remove it?
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.
In various intermediate versions of the PR, it wasn't possible, so I eliminated it. Later on, I didn't see an obvious need to reimplement it. Plus, the only usage of it in Base (in serialize) appears to be a race condition.
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.
restored in 07ef29f