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

Exposing UVError doesn't seem right #7841

Closed
IainNZ opened this issue Aug 5, 2014 · 14 comments · Fixed by #28287
Closed

Exposing UVError doesn't seem right #7841

IainNZ opened this issue Aug 5, 2014 · 14 comments · Fixed by #28287
Labels
domain:error handling Handling of exceptions by Julia or the user domain:io Involving the I/O subsystem: libuv, read, write, etc. status:help wanted Indicates that a maintainer wants help on an issue or pull request
Milestone

Comments

@IainNZ
Copy link
Member

IainNZ commented Aug 5, 2014

julia> try
         run(`false`)
       catch err
         dump(err)
       end
ErrorException
  msg: ASCIIString "failed process: Process(`false`, ProcessExited(1)) [1]"

julia> try
         run(`foobar`)
       catch err
         dump(err)
       end
UVError
  prefix: ASCIIString "could not spawn `foobar`"
  code: Int32 -2

I feel like UVError isn't something that should be escaping the Julia internals. For one thing, I doubt anyone not involved with Julia development knows or wants to know what the UV refers to. Having different exception types thrown from the same method (from the users perspective) feels odd to me, although I think thats a much more debatable point.

It bit me recently (I had code like println(fp, err.msg), which threw an error in the error handler...) and I just saw #7822 which made me think of it again.

@StefanKarpinski
Copy link
Sponsor Member

Wholeheartedly agree.

@staticfloat
Copy link
Sponsor Member

We had a long talk about this today. I think our general consensus was that we definitely want granularity of errors, (e.g. not just reporting everything as a generic ErrorException even with a helpful string) correctness of naming (calling things UVError is a little too implementation-specific, but then again most of our I/O subsystem is based on libuv) so as not to confused users, and consistency across Base.

The best short-term proposal (e.g. something within scope for 0.4) I can come up with is the following: Create categories of errors that are small enough to be meaningful (e.g. IOError), and present those to the user with libuv error codes embedded, and friendly string messages given at the time of raising the exception. This has the following benefits:

  • It allows us to present something much more unified and easy-to-understand to the user. We can have exceptions that are specific to IO, and exceptions that are specific to whatever else we use libuv for, and not mix them up.
  • Users who want to catch exceptions that map to a very specific situation can check the libuv error code. Users who want to do that kind of thing are necessarily advanced enough to understand that there are such a thing as error codes, and making those available and easily digested is worthwhile.
  • If an error code maps to an automatic message that doesn't mean much to the user at first glance, (such as this one) can be overridden to something much more user-friendly. Instead of getting "Error: Connection aborted" you can get "Error: Server socket was closed while attempting to accept a client"

@JeffBezanson
Copy link
Sponsor Member

I've been complaining about UVError for some time:

#6825 (comment)
#1924 (comment)

See also #4744

@JeffBezanson
Copy link
Sponsor Member

Also, an exception type should describe a problem, not which library threw it.

@jakebolewski jakebolewski added the domain:error handling Handling of exceptions by Julia or the user label Jun 2, 2015
@StefanKarpinski StefanKarpinski added the status:help wanted Indicates that a maintainer wants help on an issue or pull request label Dec 4, 2015
@samoconnor
Copy link
Contributor

#15879

@IainNZ
Copy link
Member Author

IainNZ commented Jun 2, 2018

I think this is still relevant (the original example still works). UVError is not exported from Base, so is it correct that its OK to put this off past 1.0?

@StefanKarpinski StefanKarpinski added the status:triage This should be discussed on a triage call label Jun 4, 2018
@JeffBezanson
Copy link
Sponsor Member

Agree this should be replaced with SystemError, IOError, etc.

@JeffBezanson JeffBezanson added this to the 1.0 milestone Jun 7, 2018
@JeffBezanson JeffBezanson removed the status:triage This should be discussed on a triage call label Jun 7, 2018
@StefanKarpinski
Copy link
Sponsor Member

Shouldn't this be on the 0.7 milestone rather than 1.0?

@JeffBezanson
Copy link
Sponsor Member

Yes I think this could be on the 0.7 milestone, but it's fairly minor since code probably doesn't reference UVError too often.

@samoconnor
Copy link
Contributor

In #15879 a subset of UVError was replaced by DNSError. A quick scan of the possible UV_ error codes suggests that there is scope to continue this pattern and split out more specific error types in future (IOPermissionError?, IOBusyError?, HostUnreacableError?).

However, I'm concerned that baking IOError into 1.0 with its current meaning would preclude splitting out more fine-grained error types.

e.g if the code below was written for Julia 1.0, it would stop working if IOPermissionError was later added as a fine-grained error type for UV_EACCES.

catch e
    if e isa IOError && e.code == UV_EACCES
        bar()
    else
        rethrow(e)
    end
end

It might be better for IOError to be an abstract type so that a future IOPermissionError could be <: IOError. Then code like the following would continue to work as fine-grained error types are added:

catch e
    if e isa AbstractIOError && errorcode(e) == UV_EACCES
        bar()
    else
        rethrow(e)
    end
end

@JeffBezanson
Copy link
Sponsor Member

Ok so should we just add AbstractIOError above IOError (and EOFError and DNSError)?

@StefanKarpinski
Copy link
Sponsor Member

StefanKarpinski commented Jul 31, 2018

Technically making IOError abstract in the future is not a breaking change. Dispatch on it and fields that use it as a type would continue to work as expected. Only code that was written to the new subtypes would see any difference and that's not breaking.

@samoconnor
Copy link
Contributor

Then perhaps the IOError.code field should be renamed to uvcode (syscode? internalcode?). Otherwise future IOError subtypes can't use the field name code.
e.g. if HTTPError <: IOError then .code would be 404 not a UV_ code.

@JeffBezanson
Copy link
Sponsor Member

The code field is implementation-defined anyway; it's pretty hard to commit to a meaning for all of its possible values. HTTPError would specify that .code is an HTTP error code, but IOError says the code is implementation-defined and you don't really know what it is.

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 domain:io Involving the I/O subsystem: libuv, read, write, etc. status:help wanted Indicates that a maintainer wants help on an issue or pull request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants