-
Notifications
You must be signed in to change notification settings - Fork 9
(Too) many changes... #65
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
Conversation
|
Hmmm seems I introduced a bug in Julia 1.0 🤔 |
src/ctask.jl
Outdated
| ct = _current_task() | ||
| res = func() | ||
| ct.result = res | ||
| ct.state = :done |
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 change might be problematic during task switching, although I can't remember the exact details
maybe @KDr2 can clarify?
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.
Hmm yes, I was wondering if it is needed. I removed it (which worked fine on Julia > 1.0 it seems) since in
Lines 189 to 190 in 2ca2a89
| isa(p.storage, IdDict) && haskey(p.storage, :_libtask_state) && | |
| (p.state = p.storage[:_libtask_state]) |
ct.state is set to ct.storage[:_libtask_state] if it exists. But maybe it's actually 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.
I'll check what happens if I revert the changes that you commented on.
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 seems to resolve the issue but I would still be interested to know the details - @KDr2, can you explain why this separate state is 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.
And there's still something not quite right it seems.
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.
Seems not related to Libtask, more like a Julia Windows x86 issue on the nightly build
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 would still be interested to know the details
Any copy of a task should NOT finish or fail before the original task finishes or fails. So I added a wait() at the end of each branch in the function task_wrapper, and that makes the tasks and their copies never be done... It's a bad solution but I couldn't find a better way to ensure a task don't end before its origin end.
If we assign :done or :failed directly to task.state, the task ends, and this may happen before its origin ends.
The root reason is how Julia starts and ends a task (pseudo code):
jl_task_t *t = get current task; // (a)
t->run_the_julia_code(); // (b) here, we copy the task
finish_and_clean(t); // (c)
a. t is the original task
b. origin and copies. Let's assume a copy (say, task_ct) ends first, then the control flow goes to (c)
c. we come back to here because a copy (task_ct) ends, but the original task (t) doesn't ends yet, but t is the original task and we do clean job on it...
There are our ways to fix this:
- at the (c) point, we refresh the variable
t(t = get_current_task();) then clean it. this solution is the perfect one but it should modify the code of JuliaLang... - do some hack, update the t variable (which is on the stack) in a copied task, this is hard and arch specific and not portable...
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.
thanks, @KDr2. As @devmotion mentioned, removing this hack seems working fine for Julia versions from v1.1. It would be interesting to understand why. Also, can you add some notes to this hack in a separate PR for future reference?
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.
OK, I will do some investigation and write some docs. But I think it must don't work under Julia >1.1 without the hack neither since I just looked into relevant code in JuliaLang today, maybe the bug is not triggered by current test cases. Anyway, I will write this down and make it clean how it works and how it doesn't work.
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 any case, thanks for the detailed explanations!
KDr2
left a comment
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 had a closer look at the implementation due to #64 and ended up with a huge amount of changes, so it might be better to split this PR...
Basically, it includes:
CTaskwrapper forTasks for which task copying is enabledCTaskthat just consumesCTaskException@assertwith@testTRef@testset).