add `TaskFailedException` to propagate backtrace of failed task in `wait` #32814

JeffBezanson · 2019-08-07T05:18:11Z

Before:

julia> t = @async rand(2)[3]; wait(t)
ERROR: BoundsError: attempt to access 2-element Array{Float64,1} at index [3]
Stacktrace:
 [1] try_yieldto(::typeof(Base.ensure_rescheduled), ::Base.RefValue{Task}) at ./task.jl:552
 [2] wait() at ./task.jl:609
 [3] wait(::Base.GenericCondition{Base.Threads.SpinLock}) at ./condition.jl:107
 [4] wait(::Task) at ./task.jl:214
 [5] top-level scope at REPL[1]:1

After:

julia> t = @async rand(2)[3]; wait(t)
ERROR: FailedTaskException:
BoundsError: attempt to access 2-element Array{Float64,1} at index [3]
Stacktrace:
 [1] getindex at ./array.jl:742 [inlined]
 [2] (::getfield(Main, Symbol("##3#4")))() at ./task.jl:332
Stacktrace:
 [1] wait(::Task) at ./task.jl:251
 [2] top-level scope at REPL[1]:1

So the location of the failure inside the waited-for task is now visible.

~~I'm not sure if a CapturedException is the best way to do this, but it was within easy reach. Maybe it would make sense to use the exception stack somehow? @c42f~~

c42f · 2019-08-07T08:00:38Z

Interesting. Doing it this way with CapturedException is nice because it's simple. But it can definitely loose the root cause in some cases.

Thinking out loud, one option would be to import the exception stack from the failed task into the current task. But that seems quite confusing because there's no way to tell which exceptions arose on which task.

Another option is to have a FailedTaskException(t::Task) which wraps the failed task itself. Then the entire root cause would be available and could be reported using Base.catch_stack(t). This seems clear and no less annoying to pattern match the nested exceptions inside.

Going further, it would be nice if wait() and Base.sync_end() were consistent — at least if we want wait() to allow waiting on multiple tasks. In that case a CompositeException wrapping some FailedTaskException seems like the consistent choice. The python trio people have more or less the same issue with their MultiError (python-trio/trio#611) and they've concluded that MultiError should always be thrown even when only a single child task fails. I tend to agree with the reasoning there. See also C#'s AggregateException.

JeffBezanson · 2019-08-07T14:33:27Z

Another option is to have a FailedTaskException(t::Task) which wraps the failed task itself.

+1 This sounds good.

c42f

This looks good to me. Needs a couple of tests for show of FailedTaskException.

c42f · 2019-08-08T04:12:19Z

base/task.jl

+    while isa(ex.task.exception, FailedTaskException)
+        pushfirst!(stacks, ex.task.backtrace)
+        ex = ex.task.exception
+    end


Ok, this surfaces the underlying error through multiple levels of wait(). That's nice:

julia> t1 = @async sum() t2 = @async wait(t1) wait(t2) ERROR: FailedTaskException: MethodError: no method matching sum() Closest candidates are: sum(::Tuple{Any,Vararg{Any,N} where N}) at tuple.jl:379 sum(::StepRangeLen) at twiceprecision.jl:536 sum(::AbstractRange{#s66} where #s66<:Real) at range.jl:978 ... Stacktrace: [1] (::getfield(Main, Symbol("##23#24")))() at ./task.jl:332 Stacktrace: [1] wait(::Task) at ./task.jl:251 [2] (::getfield(Main, Symbol("##25#26")))() at ./task.jl:332 Stacktrace: [1] wait(::Task) at ./task.jl:251 [2] top-level scope at REPL[5]:3

stdlib/Distributed/test/distributed_exec.jl

stdlib/Serialization/test/runtests.jl

test/worlds.jl

c42f · 2019-08-08T04:34:45Z

base/task.jl

-                rethrow()
+        if isa(r, Task)
+            _wait(r)
+            if istaskfailed(r)


Nice improvement not having the extra catch and rethrow.

StefanKarpinski · 2019-08-08T19:44:17Z

NEWS.md

@@ -37,6 +37,8 @@ New library functions
 Standard library changes
 ------------------------

+* When `wait` (or `@sync` etc.) is called on a failing `Task`, the exception is propagated as a
+  `FailedTaskException` wrapping the task ([#32814]).


You might want a few words about why this is an improvement (i.e. you no longer lose the backtrace).

(should also mention fetch in the docs)

vtjnash · 2019-08-08T19:46:50Z

base/task.jl

+    var = esc(sync_varname)
+    quote
+        local ref = $(esc(expr))
+        if $(Expr(:isdefined, var))


this should perhaps not include the isdefined check and just assert that there is a sync pool?

JeffBezanson · 2019-08-08T20:21:30Z

Hm, now I notice we added ProcessFailedException. So maybe the name should be swapped around to TaskFailedException?

c42f · 2019-08-08T21:28:02Z

Hmm ProcessFailedException was #27900 (CC @oxinabox). It's very similar in intent though it can hold multiple failed processes rather than using CompositeException. If we wanted one exception type to do double duty what would we call it? TaskFailedException seems confusing if the wrapped object happened to be a Process. Move right along nothing to see here 🤦‍♂️

JeffBezanson · 2019-08-08T21:29:15Z

I'm not saying to combine them, just use the same word order: ProcessFailedException and TaskFailedException, instead of ProcessFailedException and FailedTaskException.

StefanKarpinski · 2019-08-08T22:07:28Z

+1 to having the word order match: ProcessFailedException and TaskFailedException.

vchuravy · 2019-08-09T07:42:26Z

base/task.jl

-            if !isa(r, Task) || (isa(r, Task) && !istaskfailed(r))
-                rethrow()
+        if isa(r, Task)
+            _wait(r)


x-ref: #32677

If the first task is blocking on a second task and the second task is throwing an exception, we will never see the exception and @sync will block forever.

👍 but out of scope for this PR.

oxinabox · 2019-08-09T09:55:52Z

test/channels.jl

@@ -323,12 +323,12 @@ end
    ct = current_task()
    testerr = ErrorException("expected")


Probably out of scope for this PR,
but do you think
we should have a custom error type defined in Test stdlib for testing these things,
so it is harder to get false positives from other errors being thrown?

(If so i can open an issue)

…ait`

…ait` (#32814)

JeffBezanson added error handling Handling of exceptions by Julia or the user multithreading Base.Threads and related functionality labels Aug 7, 2019

JeffBezanson force-pushed the jb/taskbt branch from 99edd08 to 6acf497 Compare August 7, 2019 20:08

JeffBezanson changed the title ~~propagate backtrace of failed task in wait~~ RFC: add FailedTaskException to propagate backtrace of failed task in wait Aug 7, 2019

c42f approved these changes Aug 8, 2019

View reviewed changes

JeffBezanson added the triage This should be discussed on a triage call label Aug 8, 2019

StefanKarpinski added the minor change Marginal behavior change acceptable for a minor release label Aug 8, 2019

StefanKarpinski reviewed Aug 8, 2019

View reviewed changes

vtjnash reviewed Aug 8, 2019

View reviewed changes

JeffBezanson force-pushed the jb/taskbt branch from 3ac876c to 79bf746 Compare August 8, 2019 22:14

JeffBezanson changed the title ~~RFC: add FailedTaskException to propagate backtrace of failed task in wait~~ RFC: add TaskFailedException to propagate backtrace of failed task in wait Aug 8, 2019

vchuravy reviewed Aug 9, 2019

View reviewed changes

oxinabox reviewed Aug 9, 2019

View reviewed changes

JeffBezanson mentioned this pull request Aug 9, 2019

UndefRefError with @threads for multithreading on 1.3 #32846

Closed

JeffBezanson changed the title ~~RFC: add TaskFailedException to propagate backtrace of failed task in wait~~ add TaskFailedException to propagate backtrace of failed task in wait Aug 9, 2019

add TaskFailedException to propagate backtrace of failed task in `w…

5e6a91a

…ait`

JeffBezanson force-pushed the jb/taskbt branch from 79bf746 to 5e6a91a Compare August 12, 2019 19:30

JeffBezanson merged commit df1ee8c into master Aug 13, 2019

JeffBezanson deleted the jb/taskbt branch August 13, 2019 18:25

JeffBezanson removed the triage This should be discussed on a triage call label Aug 13, 2019

mortenpi mentioned this pull request Aug 14, 2019

Fix tests on nightly JuliaDocs/Documenter.jl#1096

Merged

c42f mentioned this pull request Aug 19, 2019

asyncmap: Include original backtrace in rethrown exception #32749

Closed

Keno pushed a commit that referenced this pull request Jun 5, 2024

add TaskFailedException to propagate backtrace of failed task in `w…

44d54fe

…ait` (#32814)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add `TaskFailedException` to propagate backtrace of failed task in `wait` #32814

add `TaskFailedException` to propagate backtrace of failed task in `wait` #32814

JeffBezanson commented Aug 7, 2019 •

edited

Loading

c42f commented Aug 7, 2019

JeffBezanson commented Aug 7, 2019

c42f left a comment

c42f Aug 8, 2019

c42f Aug 8, 2019

StefanKarpinski Aug 8, 2019

NHDaly Aug 8, 2019

vtjnash Aug 8, 2019

JeffBezanson Aug 8, 2019

JeffBezanson commented Aug 8, 2019

c42f commented Aug 8, 2019 •

edited

Loading

JeffBezanson commented Aug 8, 2019

StefanKarpinski commented Aug 8, 2019

vchuravy Aug 9, 2019

JeffBezanson Aug 9, 2019

oxinabox Aug 9, 2019 •

edited

Loading

		@@ -323,12 +323,12 @@ end
		ct = current_task()
		testerr = ErrorException("expected")

add TaskFailedException to propagate backtrace of failed task in wait #32814

add TaskFailedException to propagate backtrace of failed task in wait #32814

Conversation

JeffBezanson commented Aug 7, 2019 • edited Loading

c42f commented Aug 7, 2019

JeffBezanson commented Aug 7, 2019

c42f left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffBezanson commented Aug 8, 2019

c42f commented Aug 8, 2019 • edited Loading

JeffBezanson commented Aug 8, 2019

StefanKarpinski commented Aug 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxinabox Aug 9, 2019 • edited Loading

Choose a reason for hiding this comment

add `TaskFailedException` to propagate backtrace of failed task in `wait` #32814

add `TaskFailedException` to propagate backtrace of failed task in `wait` #32814

JeffBezanson commented Aug 7, 2019 •

edited

Loading

c42f commented Aug 8, 2019 •

edited

Loading

oxinabox Aug 9, 2019 •

edited

Loading