Skip to content
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

Show full stacktrace when Channel Task throws #35177

Merged
merged 2 commits into from
Apr 30, 2020

Conversation

non-Jedi
Copy link
Contributor

@non-Jedi non-Jedi commented Mar 19, 2020

Not sure if this counts as a feature or a bug-fix. This was a major
pain-point for a user on gitter which seemed an easy fix once I
understood the system.

This reuses the machinery that gives good stacktraces when a user
calls wait on a Task. Since bind internally uses _wait2, this
machinery is bypassed currently.

Currently, julia throws an uninformative stacktrace in this situation:

julia> c = Channel(_ -> error("foo"))
Channel{Any}(sz_max:0,sz_curr:0)

julia> for i in c end
ERROR: foo
Stacktrace:
 [1] iterate(::Channel{Any}) at ./channels.jl:459
 [2] top-level scope at ./REPL[2]:1

With this change, the stacktrace is much more informative:

julia> for i in c end
ERROR: TaskFailedException:
foo
Stacktrace:
 [1] error(::String) at ./error.jl:33
 [2] (::var"#1#2")(::Channel{Any}) at ./REPL[4]:1
 [3] (::Base.var"#652#653"{var"#1#2",Channel{Any}})() at ./channels.jl:129
Stacktrace:
 [1] check_channel_state at ./channels.jl:167 [inlined]
 [2] take_unbuffered(::Channel{Any}) at ./channels.jl:405
 [3] take! at ./channels.jl:383 [inlined]
 [4] iterate(::Channel{Any}, ::Nothing) at ./channels.jl:449
 [5] iterate(::Channel{Any}) at ./channels.jl:448
 [6] top-level scope at ./REPL[5]:1

This reuses the machinery that gives good stacktraces when a user
calls wait on a Task. Since bind internally uses _wait2, this
machinery is bypassed currently.

Currently, julia throws an uninformative stacktrace in this situation:

julia> c = Channel(_ -> error("foo"))
Channel{Any}(sz_max:0,sz_curr:0)

julia> for i in c end
ERROR: foo
Stacktrace:
 [1] iterate(::Channel{Any}) at ./channels.jl:459
 [2] top-level scope at ./REPL[2]:1

With this change, the stacktrace is much more informative:

julia> for i in c end
ERROR: TaskFailedException:
foo
Stacktrace:
 [1] error(::String) at ./error.jl:33
 [2] (::var"JuliaLang#1#2")(::Channel{Any}) at ./REPL[4]:1
 [3] (::Base.var"JuliaLang#652#653"{var"JuliaLang#1#2",Channel{Any}})() at ./channels.jl:129
Stacktrace:
 [1] check_channel_state at ./channels.jl:167 [inlined]
 [2] take_unbuffered(::Channel{Any}) at ./channels.jl:405
 [3] take! at ./channels.jl:383 [inlined]
 [4] iterate(::Channel{Any}, ::Nothing) at ./channels.jl:449
 [5] iterate(::Channel{Any}) at ./channels.jl:448
 [6] top-level scope at ./REPL[5]:1
@non-Jedi
Copy link
Contributor Author

Any comments on this or things I need to fix? If this isn't considered too breaking of a change, I believe this is ready to merge.

@non-Jedi
Copy link
Contributor Author

non-Jedi commented Apr 30, 2020

Bumping this per fredrikekre's post on discourse in case it can make it into 1.5. I understand if it can't.

@StefanKarpinski
Copy link
Member

This can still make it into 1.5. Tagging for discussion on the triage call today.

@StefanKarpinski StefanKarpinski added triage This should be discussed on a triage call and removed triage This should be discussed on a triage call labels Apr 30, 2020
@JeffBezanson JeffBezanson merged commit 1475273 into JuliaLang:master Apr 30, 2020
non-Jedi added a commit to non-Jedi/julia that referenced this pull request Apr 30, 2020
This was a simplification missed in original JuliaLang#35177
@non-Jedi
Copy link
Contributor Author

I just noticed that I missed a braindead simplification in this PR and pushed e17696e to make this code actually make sense to people unaware of its history.

@StefanKarpinski
Copy link
Member

I don't see a PR for that yet. Am I missing it or is it yet to be opened?

@non-Jedi
Copy link
Contributor Author

non-Jedi commented Apr 30, 2020

I pushed it to the branch for this PR but can open a new one. Looks like CI doesn't get run if you push to a branch after it's merged, so I'll go ahead and open a new PR.

EDIT: #35673

non-Jedi added a commit to non-Jedi/julia that referenced this pull request Apr 30, 2020
This was a simplification missed in original JuliaLang#35177
JeffBezanson pushed a commit that referenced this pull request May 1, 2020
This was a simplification missed in original #35177
@NHDaly
Copy link
Member

NHDaly commented Sep 10, 2020

Thanks for this, @non-Jedi! :) This is great - we just noticed this was missing in 1.4 and started drafting a fix before noticing you'd already fixed it on 1.5 😊 👏 👍

@non-Jedi
Copy link
Contributor Author

Glad this was helpful to y'all. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants