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

WIP: Uncatchable fatal errors #15906

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

samoconnor
Copy link
Contributor

@samoconnor samoconnor commented Apr 17, 2016

See #15514

Interface

try causes ordinary errors to disappear as usual:

julia> try error("foo") end

try no longer catches fatal errors:

julia> try foo end
ERROR: UndefVarError: foo not defined
 in eval(::Module, ::Any) at ./boot.jl:236

The isfatal function decides which errors are fatal:

isfatal(error) = false
isfatal(::StackOverflowError) = true
isfatal(::OutOfMemoryError) = true
isfatal(::UndefVarError) = true

Handling of fatal exceptions can be reenabled for a particular stack frame (e.g. for the REPL, @test_throws, RPC, etc...):

julia> function f()
           Base.enable_catch_fatal()
           try foo catch ex println("Caught: $ex") end
           Base.disable_catch_fatal()
           bar
       end

julia> f()
Caught: UndefVarError(:foo)
ERROR: UndefVarError: bar not defined

Implementation

A call to rethrow_if_fatal is inserted at the top of the catch block:

julia> expand(:(try foo end))

:($(Expr(:thunk, Toplevel LambdaInfo thunk
:(begin
        $(Expr(:enter, 6)) # REPL[1], line 1:
        GenSym(0) = foo
        $(Expr(:leave, 1))
        return GenSym(0)
        6:
        $(Expr(:leave, 1))
        (top(rethrow_if_fatal))($(Expr(:the_exception)))
        return
    end))))

rethrow_if_fatal() rethrows fatal exceptions unless, e.g., the REPL wants to catch fatal exceptions at a particular stack frame.

rethrow_if_fatal(error) = isfatal(error) && ccall(:jl_rethrow_fatal, Void, ())

The REPL calls Base.enable_catch_fatal() in eval_user_input. This allows it to catch and display fatal errors as usual.

@tkelman
Copy link
Contributor

tkelman commented Apr 17, 2016

How could adding a check to the top of every catch ever possibly make throwing errors more efficient, if that's a possible future advantage mentioned in #15514? This also makes throwing these exception types no longer testable via @test_throws, so that would have to be fixed.

I strongly dislike opening the door to packages defining their exceptions as fatal in this uncatchable way without any clean workarounds (aside from trying to overwrite those methods, which can be unreliable with inlining). Any exception type where isfatal returns true is unusable inside higher-level code that does backtracking or exploratory algorithms that need to be able to recover even from exceptional circumstances that some utility package's author may consider to be bugs. Exceptions as control flow shouldn't be a common or widely used idiom if it can be avoided, but there are use cases where it can't. This type of code is also a counterexample to the opinion some may have about performance not mattering if an exception is being thrown.

rethrow_if_fatal(error) = isfatal(error) && ccall(:jl_rethrow_fatal, Void, ())
else
rethrow_if_fatal(error) = nothing
end
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rethrow_if_fatal is disabled for coreimg.jl for now because it causes the following error in the first pass of inference.jl compilation:

inference.jl
error during bootstrap:
UndefVarError(var=:Base)
rec_backtrace at /Users/sam/git/octech/julia/julia/src/stackwalk.c:83

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Analysis:

The try/catch in pure_eval_call ended up calling to promote_to_supertype, which was calling error("no promotion exists for ", Float16, " and ", Int64). However, error was defined as throw(Main.Base.ErrorException(Main.Base.string(s...))), and Base does not exist when building coreimg.jl, so the result was UndefVarError(var=:Base).

Fix:

Define a simple version of error that works without Base:

    type CoreErrorException <: Exception
        msg
    end
    error(s...) = throw(CoreErrorException(s))

@samoconnor
Copy link
Contributor Author

Joe Duffy wrote...

Abandonment, and the degree to which we used it [in Midori], was in my opinion our biggest and most successful bet with the Error Model. We found bugs early and often, where they are easiest to diagnose and fix. Abandonment-based errors outnumbered recoverable errors by a ratio approaching 10:1 ...

Stefan wrote...

I agree that uncatchable exceptions make sense

Jeff wrote...

I do like the idea of something like this. The idea of catching, say, an UndefVarError is crazy enough to warrant some special treatment at the language level.

The implementation in this PR passing now passes all the CI tests.

@JeffBezanson, @StefanKarpinski: any comments on the isfatal interface?

Note: So far this feature has found one place in Base where a catch was hiding a bug:
The try/catch in pure_eval_call ended up calling to promote_to_supertype, which was calling error("no promotion exists for ", Float16, " and ", Int64). However, error was defined as throw(Main.Base.ErrorException(Main.Base.string(s...))), and Base does not exist when building coreimg.jl, so the resulting error was UndefVarError(var=:Base) instead of ErrorException. This PR makes UndefVarError uncatchable which uncovered the problem.

@nalimilan
Copy link
Member

nalimilan commented Apr 18, 2016

Note: So far this feature has found one place in Base where a catch was hiding a bug:
The try/catch in pure_eval_call ended up calling to promote_to_supertype, which was calling error("no promotion exists for ", Float16, " and ", Int64). However, error was defined as throw(Main.Base.ErrorException(Main.Base.string(s...))), and Base does not exist when building coreimg.jl, so the resulting error was UndefVarError(var=:Base) instead of ErrorException. This PR makes UndefVarError uncatchable which uncovered the problem.

This bug would have equally been detected if try...catch only caught a specified exception type (here ErrorException) instead of all types. No need to introduce fatal exceptions for that.

@samoconnor
Copy link
Contributor Author

@nalimilan I agree. I have been advocating for catch to only catch specified types for some time in various issues and PRs. I've even built a package to do that: https://github.com/samoconnor/Retry.jl (used extensively in AWS*.jl).
However, it seems that agreeing on and implementing something like what Stefan has in mind is likely to take some time. In the meantime, this PR provides a very simple way to reduce bugs today.

I have read your comments in #15514 to the effect that a properly designed try/catch annotation system would make a special mechanism for abandonment-on-bugs unnecessary. I think you are probably right, and I hope that we get there sooner rather than later. I have a feeling that Joe Duffy's explicit differentiation bugs and exceptions has merit and should not be set aside lightly. However, it could be that a unified implementation handles both and the distinction only has to be made through documentation and API design conventions.

@kshyatt kshyatt added the error handling Handling of exceptions by Julia or the user label Jul 3, 2016
@kshyatt
Copy link
Contributor

kshyatt commented Jul 3, 2016

Can we get a rebase on this? Is this something we're still interested in doing?

@samoconnor
Copy link
Contributor Author

I'm happy to spend the time to rebase if someone with commit access is interested in reviewing and merging this...

@eschnett
Copy link
Contributor

eschnett commented Jul 4, 2016

This feature will become interesting if we can throw fatal exception early, or delay throwing them, or throw them out of order, because this will allow re-arranging code during optimization. (For example, one could change the order in which a loop is traversed, and indexing errors might then be reported for a different (later) index than without such a reordering.)

Is that possible with this design?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
error handling Handling of exceptions by Julia or the user
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants