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

Change Errno into a hierarchy of OSError subclasses #5003

Closed
wants to merge 5 commits into from

Conversation

oprypin
Copy link
Member

@oprypin oprypin commented Sep 18, 2017

Initial exception hierarchy is:

  • OSError (former Errno)
    • OSError::BlockingIO {EAGAIN, EALREADY, EINPROGRESS, EWOULDBLOCK}
    • OSError::FileExists {EEXIST}
    • OSError::FileNotFound {ENOENT}
    • OSError::IsADirectory {EISDIR}
    • OSError::NotADirectory {ENOTDIR}
    • OSError::PermissionError {EACCES, EPERM}
    • ConnectionError
      • BrokenPipe {EPIPE}
      • ConnectionAborted {ECONNABORTED}
      • ConnectionRefused {ECONNREFUSED}
      • ConnectionReset {ECONNRESET}

These are based on what Python does.

The best showcase of improvement is src/ecr/process.cr.

Note that I did not get rid of the inherent dependency on errno/GetLastError, because it's just not viable to predict every possible error from those countless error codes, every time an OS API is called. OSError.create substitutes the exception class with some subclass of OSError, when such a class is defined, or otherwise the generic OSError (just like Errno before) with the error code available. This is also what Python does.

Windows errors are integrated into this; see windows_error.cr.

Copy link
Contributor

@ysbaddaden ysbaddaden left a comment

Choose a reason for hiding this comment

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

This is the kind of change that means to abstract system details and hide them to the developer, but those details are very hard to hide, and in the case of Errno or WinError: impossible to.

For example moving Errno to Crystal::System means that developers musn't use Errno... but developers may need to access Errno in perfectly valid scenarios, for example when writing C bindings that for libraries that will set errno.

Same for WinError, which by-the-way should use the windows naming (WinError) not something else (WindowsError).

If we're abstracting the platform error systems, then mapping WinError -> Errno should be replaced with a mean to raise crystal exceptions.

Last but not least I really don't like the chosen python inspired naming. I'd really suggest to have per namespace exceptions, for example System::Error instead of OSError, or File::Not found, IO::BrokenPipe, ... that is, names that fit better in Crystal land.

Errno.value = @value
raise Errno.new "..."
Crystal::System::Errno.value = @value
raise OSError.create "..."
Copy link
Contributor

Choose a reason for hiding this comment

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

What?! Using Crystal::System::Errno makes no sense: it's not a platform abstraction :/

Using a ... error message is also, huh...

Copy link
Contributor

Choose a reason for hiding this comment

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

Furthermore pushing Errno as Crystal::System::Errno means that developers of C bindings will now need to use Crystal::System... but developers musn't.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well with the namespace I guess I see what you mean. But Crystal cannot provide access to errno in a cross-platform way because it's a UNIX-specific thing, and at the same time, Crystal::System::Errno is, in fact, a platform abstraction (different POSIX platforms). What do you suggest?
By the way, developers technically don't have to access Crystal::System::Errno, they can just use OSError.create.errno, but they usually shouldn't need to.

Copy link
Member Author

Choose a reason for hiding this comment

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

Reworked this. But now accessing errno is a compilation error on non-POSIX.
at least I think so...

@asterite
Copy link
Member

I agree with the spirit of this change. Every other high-level programming language (except Ruby) abstracts Errno with meaningful errors (for example Java, C#, Python andGo). Being able to access the value of errno is also good because of the reasons explained by @oprypin . I also remember @waj wanted to get rid of the Errno exception type. So 👍 from me (after reviewing and making the necessary corrections).

@oprypin
Copy link
Member Author

oprypin commented Sep 18, 2017

@ysbaddaden, I renamed the exceptions slightly.

by-the-way should use the windows naming (WinError)

Strongly disagree that naming in Windows headers should in any way affect naming in Crystal.

@molovo
Copy link
Contributor

molovo commented Sep 18, 2017

This is a great change, should make life a lot easier. Nice work @oprypin

I do like @ysbaddaden's idea of namespacing the errors, but I don't think they belong directly in the namespace. Perhaps a good convention would be to have an Error namespace which they could live in so that it's immediately obvious from the name what it is. More like File::Error::NotFound, IO::Error::BrokenPipe etc.

@RX14
Copy link
Contributor

RX14 commented Sep 18, 2017

I agree with @ysbaddaden that OSError's subclasses should live in the namespaces which raise these errors, instead of the OSError namespace. The best way to design this is to forget about the OS and think about "if my OS and stdlib was written in crystal, how would I design this", then think about how best to map this to crystal and make compromises if needed. People using these exceptions should very rarely need to think that these errors are "os errors", they're just a normal error which is raised from File for example, so it should live in the File namespace.

@oprypin
Copy link
Member Author

oprypin commented Sep 19, 2017

@RX14, that last sentence is definitely disjointed from reality. OSError is special in the way that any system call can produce any of them. Scattering them across the standard library will not help anyone, and is a huge source of bikeshedding.

@oprypin
Copy link
Member Author

oprypin commented Sep 19, 2017

Please present a concrete agreed upon set of names, and I can accept it
(please no System::Error though)

@RX14
Copy link
Contributor

RX14 commented Sep 19, 2017

@oprypin Except not all syscalls produce all errnos? If the same errno has a different enough semantic meaning in two different syscalls it should become 2 different crystal error types. I am in favour of complete abstraction. If people want to think in terms of errnos, they can simply catch OSError and check the errno on that. 99% of people do not want to know or care about syscalls, and so we should design the exception hierarchy entirely without them in mind. I'm sure we'll need to make some compromises, but I want to stick as close to that ideal as possible.

@oprypin
Copy link
Member Author

oprypin commented Sep 19, 2017

@RX14, as I've said before, if we are relying on syscalls at all, it's an extreme amount of work trying to predict all possible error codes for every case. There would still probably need to be a fallback error anyway.
This change is simple and non-disruptive. It does not make full abstraction any harder to achieve than before, but is a big improvement by itself. Beggars can't be choosers. I consider this comment not constructive.

@oprypin
Copy link
Member Author

oprypin commented Sep 19, 2017

In the next 2 days I will not be able to edit the code of this pull request, but I welcome comments that have a concrete idea of implementation, not just pointing out a problem.

It would be great to get an agreement on the naming of exceptions, so maybe someone can suggest a complete set of names and people can vote on that.

Also tagging @txe -- maybe you have some input.

@oprypin
Copy link
Member Author

oprypin commented Sep 19, 2017

If the same errno has a different enough semantic meaning in two different syscalls it should become 2 different crystal error types.

This PR provides a sane default for every possible case without any work needed. If some undesired behavior is detected, it is easy to add a check on a case-by-case basis and manually create a different exception type (and even keep the original errno in it which is nice).

@RX14
Copy link
Contributor

RX14 commented Sep 19, 2017

I agree - this is a massive improvement - but i'm always wary of merging imperfect improvements because I'm scared they'll become "good enough" to tag 1.0 and then we'll never get a chance to improve them further. I think that's unreasonable of me. I'll add improving this to the roadmap so we don't forget it before 1.0.

@txe
Copy link
Contributor

txe commented Sep 19, 2017

I suppose there is a misunderstanding of what @RX14 is trying to say. I'll try to say the same in other words.
There is an idea of unifying of using exceptions across aboard. For example, class Base64 can raise exceptions BadEncoding, BadDecoding and they are not related to errno. Suppose, there are many classes with their exception then It will be good idea to keep these exceptions in the namespace of that classes because only these classes will raise their exceptions.
So, this idea can by applied to class File (in spite of the fact the exceptions have the errno attriute) otherwise a user will feel there is something special in case of File although there is no such thing.
Also, it doesn't mean there is a need to cover all errno.

@konovod
Copy link
Contributor

konovod commented Sep 19, 2017

I think is that syscallls should raise something like OsError, but stdlib methods should try to intercept them and reraise as a File::ENotFound etc - because methods in stdlib will have enough context to identify what exactly this EACCES mean. I'm not expert though, maybe this intermediate level is overkill.

@ysbaddaden
Copy link
Contributor

Please note that I previously proposed a solution to have per-errno exception classes, which was rejected partly because it noticeably impacted the compiler performance, despite however helpful and nicer it was to raise and catch specific errno.

Compiler performance should be verified here too.

@oprypin bloating the global or OSError namespace with lots of exception classes is a bad design. If we don't care about Errno and want to introduce cross platform intelligible namings it MUST be done in a logical API design.

As RX14 suggests, how would we design and name the exception if we didn't make a syscall? Would we introduce a ::FileNotFoundError? Or would we introduce a File::NotFound?

Otherwise, it's just hiding some well-known naming (Errno, WinError) for another naming (OSError), it's introducing a bad API design, ...

That being said, we can't catch and verify each possible errno value after each syscall, this is tedious and will bloat the stdlib, but we can check the most important one(s) for each syscall, even maybe have a generic exception raiser for a namespace, and default to a generic exception for other values.

Last but not least, I wouldn't hide Errno.value (it's cross platform).

@oprypin
Copy link
Member Author

oprypin commented Sep 19, 2017

how would we design and name the exception if we didn't make a syscall?

First of all, this theoretically does not happen, but if it does, you reuse the appropriate OSError subclass.

MUST be done in a logical API design.

So what's not logical here? Python devs spent a lot of time designing this hierarchy; it's not a historic leftover.

Last but not least, I wouldn't hide Errno.value (it's cross platform).

It's not really cross-platform. Also it's not hidden by this PR.
Also note that alternative suggestions do, in fact, hide it.

we can check the most important one(s) for each syscall, even maybe have a generic exception raiser for a namespace, and default to a generic exception for other values.

That's what is happening but the namespace is OSError. Can't be anything else because then errno is gone.

is a bad design

introducing a bad API design

Might as well move on to ad hominem. Not sure if those positive votes reflect that though

@straight-shoota
Copy link
Member

I hope the last weeks have given some time to look at this discussion with some distance.

@oprypin I don't think @ysbaddaden is in any way aggressive against you. If your proposal was (in part) criticized as "bad design" it was done so with reason. Still, others can have different opinions and I think we should talk about arguments for both.

As I understand it, both approaches have good points and at the current time I couldn't make out a conclusive argument about preferring one over the other.

I think it's worth taking a look at the reasoning behind Pythons exception hierarchy. But it is worth noting that this proposal was based on an existing hierarchy with flaws. They couldn't design the best API they could think of, but had to avoid any radical changes which would break compatibility considerably.

Some general thoughts on naming exceptions:

  • Exception classes should have Exception (or Error, see next) in the name to make them easily identifyable. For example OptionParser::InvalidOption could be understood as holding an option with invalid value, IO::Timeout could be a time span. OptionParser::InvalidOptionError and IO::TimeoutError are not much more to write but improve understanding.
  • Some classes are named Error and some Exception (e.g. OptionParser::Exception, Key::Error). While it might make sense to use this to differentiate between severities (see for example Differences between Exception and Error (Java)), the naming of Crystal's exception classes doesn't seem to be conclusive at all.
  • It can be misleading to have many exception classes named Error in modules (like IO::Error). If, from inside the module, the short path without the module prefix is used to reference this exception class, it becomes ambiguous. You can't simply grep over the code to determine where a certain exception is raised. Everything is fine if for classes named Error the full name is used everywhere.

Looking at the current and proposed hierarchies I wonder about how to align things together:

There is IO::Error which - as I understand it - should represent all errors raised from the IO module / raised from IO operations. It has a subclass IO::EOFError - a specific type of error. However, there is also IO::Timeout which is an exception in IO and obviously IO-related, but not a
subtype of IO::Error. Then there is also Socket::Error. While Socket is not part of IO, it would still be reasonable to assume that an error in a socket is an IO error and that rescue IO::Error doesn't ignore malfunctioning sockets.

Many of the error types introduced by this PR classify as IO errors - independent of how they are named. If it is OSError::BlockingIO or IO::BlockingError, it would make sense to rescue it as IO::Error. Yet obviously, all classes for specific OS errors derived from errno/winerror should inherit from one base class OSError (or whatever the name). But they can't simultaneously inherit IO:Error and OSError. And
IO::Error can't inherit from OS::Error because subclasses like IO::EOFError are not OS errors.

Has anyone thought about this? Or am I in some way mistaken?

@oprypin
Copy link
Member Author

oprypin commented Oct 10, 2017

A random idea passing by: OSError should be a module

Also: isn't reading the global "last OS error" variable horribly breaking thread safety?

@RX14
Copy link
Contributor

RX14 commented Oct 10, 2017

I agree, the current exception hierarchy is badly cobbled together and has very clearly grown over time with little design.

OSError being a module is a good idea. I'd like to standardize on Error vs Exception. The base class is called Exception so perhaps naming all exceptions ending in Exception would be a good idea. On the other hand, perhaps not all exceptions are errors, so naming actual errors Error would be better. Off the top of my head, Error is winning and much more common.

@oprypin
Copy link
Member Author

oprypin commented Oct 10, 2017

All actual errors should end with Error -- and that's really all of the exceptions that you normally think about.

Again I really like what Python does. The only non-Error exceptions are special control flow exceptions:

  • SystemExit -- exit() is implemented by raising this.
  • KeyboardInterrupt -- raised on Ctrl+C signal.
  • GeneratorExit and StopIteration -- iterators are implemented by raising this when they run out of items.

But Python also prevents you from accidentally catching those because they derive from BaseException while errors derive from Exception.

@bararchy
Copy link
Contributor

bararchy commented Apr 8, 2018

@RX14 @straight-shoota @ysbaddaden ping on this? Would love to get the errorno away

@oprypin
Copy link
Member Author

oprypin commented Apr 8, 2018

I think this is basically rejected

@straight-shoota
Copy link
Member

straight-shoota commented Apr 8, 2018

While this PR has received mixed feedback, I think everyone agrees on the principal idea to replace the generic Errno/WinError with context-specific error types. But we need to work out the exact design of the exception hierarchy and agree on naming.

This PR is already pretty much cluttered, outdated and probably won't be merged anyway. So, maybe we should take a step back and continue discussion in a new issue about exception design instead of this PR.

But I think we have made some promising progress and can continue this matter on a good basis.

#5566 should maybe be resolved first - or included in a general discussion about Crystal's exception hierarchy?

@oprypin
Copy link
Member Author

oprypin commented Oct 10, 2019

woops

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.

None yet

10 participants