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

Exceptions from __init__() being caught #12505

Closed
jdlangs opened this issue Aug 7, 2015 · 15 comments
Closed

Exceptions from __init__() being caught #12505

jdlangs opened this issue Aug 7, 2015 · 15 comments
Assignees
Labels
domain:error handling Handling of exceptions by Julia or the user status:priority This should be addressed urgently

Comments

@jdlangs
Copy link
Contributor

jdlangs commented Aug 7, 2015

module X
    __init__() = error("Something went wrong")
end
WARNING: error initializing module X:
ErrorException("Something went wrong")

Why is the exception caught and just a warning message printed? I did some basic searching but could not find any reference or explanation for this. Should I not have the ability to send an error to the top-level if something goes wrong when my module is being initialized?

@ivarne
Copy link
Sponsor Member

ivarne commented Aug 9, 2015

I agree, but I haven't seen a good answer.

@ihnorton ihnorton added the domain:error handling Handling of exceptions by Julia or the user label Aug 9, 2015
@jdlangs
Copy link
Contributor Author

jdlangs commented Aug 9, 2015

It seems like it's sufficient to add a jl_rethrow(); call to this block. Is there some technical reason that won't work / will cause problems?

ETA: It also appears to work fine when the try/catch is removed entirely.

@StefanKarpinski
Copy link
Sponsor Member

@vtjnash, @timholy, @JeffBezanson – thoughts on this?

@JeffBezanson
Copy link
Sponsor Member

I think there was some desire to prevent an exception in one module's __init__ from stopping others from running. While there is a defined order of initialization, it's not very explicit --- you can't look at the code and see where the exception happened, and which __init__ functions haven't run yet.

However, when starting a standalone program, you would want an exception here to just exit(), so maybe it's fine to just remove the try/catch.

@jdlangs
Copy link
Contributor Author

jdlangs commented Aug 10, 2015

Isn't it always dangerous to convert errors into warnings, even for completely third-party code? People will inevitably slip on fixing them and the bugs will get propagated much further than they should be.

Would a possible fix be to allow using/import in a try block?

@stevengj
Copy link
Member

+1 for removing the try/catch. If my module imports another module, and the latter module refuses to load, I'd like to halt unless I explicitly put the import in a try block.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Aug 12, 2015

when importing another module, almost everything is an error (file not found, loading code causes an error, and now __init__ throws an error), except if the module doesn't actually load, that is only causes a warning:

julia> using compat
INFO: Recompiling stale cache file /Users/jameson/.julia/lib/v0.4/compat.ji for module compat.
WARNING: requiring "compat" did not define a corresponding module.

julia> using compat
WARNING: replacing module Compat
WARNING: requiring "compat" did not define a corresponding module.

that seems counter-intuitive, is rather hard to model in precompile, and goes against the general principle of avoiding warnings (and either making that something either fully allowed or fully disallowed)

@stevengj
Copy link
Member

@vtjnash, yes, I wondered about that behavior when I saw it. An exception seems better, but I thought maybe there was some case with multiple modules in the same file where a warning might be intended? But I can't think of any concrete example of this at the moment.

@StefanKarpinski
Copy link
Sponsor Member

@vtjnash – I don't understand what your position is. Are you against removing the try/catch or for it? Or are you for it and in favor of making more things that can go wrong during __init__ fatal (e.g. module doesn't actually load).

@vtjnash
Copy link
Sponsor Member

vtjnash commented Aug 12, 2015

i thought Jeff had a position that there should never be a case where emitting a warning is the intended behavior.

I think there was some desire to prevent an exception in one module's init from stopping others from running.

I think you fixed the order sufficiently so that this is not as much of a concern. When it was originally implemented, the list could contain unrelated modules so it was necessary to run through all of them, but with tkelman@506ecbe, i think it's safe to abort.

to argue the other side, however, I sometimes (with invalid build trees), get errors during startup that some library couldn't be found (such as libgmp). It's currently rather nice that it is non-fatal (although only because I am not intending to use anything from that module). Brainstorming this further however, perhaps in the future, we could some sort of "stub" module that acts as a placeholder for a real module, but which actually holds an error. This might also potentially be an improvement for the following code (among others):

> using Foo
LoadError: threw foobar

# current behavior
> using Foo
# nothing

# possible behavior?
> using Foo
ModuleDefinitionError in Foo: LoadError: threw foobar

# and also?
> Foo.bar
ModuleDefinitionError in Foo: LoadError: threw foobar

@StefanKarpinski
Copy link
Sponsor Member

Was that commit merged? If not, then why not?

@tkelman
Copy link
Contributor

tkelman commented Aug 12, 2015

That was from a funny rearranged branch I did for bisecting 10525, I'm pretty sure that commit's on master but has a different sha.

@ivarne
Copy link
Sponsor Member

ivarne commented Aug 12, 2015

An __init__ function has the ability to do whatever it likes when it can't find a library. If a warning is more apropriate, it should warn("Missing library") instead of error("Missing library").

@StefanKarpinski
Copy link
Sponsor Member

It was: 1047605. Good.

@ViralBShah ViralBShah added the status:priority This should be addressed urgently label Aug 14, 2015
@jdlangs
Copy link
Contributor Author

jdlangs commented Aug 18, 2015

Closed by #12576

@jdlangs jdlangs closed this as completed Aug 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:error handling Handling of exceptions by Julia or the user status:priority This should be addressed urgently
Projects
None yet
Development

No branches or pull requests

9 participants