-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Getting rid of Errno and WinError #8885
Conversation
I don't like having |
@straight-shoota actually I like it because it ensures uniformity. Many different new exceptions from shards or std might be created from system errors, and so many errors come from the system. I think we should properly document the |
As a compromise we could also have |
Ok, agreed. I moved the implementation to the |
At a glance, this seems strictly better than #5003 |
07ef485
to
95484a8
Compare
Thanks @oprypin. I considered some of the ideas and the feedback received on that PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for the late (post-merge) review, I have been busy recently...
I really like this change! A lot! This is close to how I always wanted Crystal's exception hierarchy. I have a few small issues we might want to change before releasing though:
end | ||
|
||
class File::Error < IO::Error | ||
getter file : String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to have some documentation on these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course. Documentation is still missing in many classes and methods.
@@ -142,7 +142,7 @@ class Process | |||
# error: | |||
errno = Errno.value | |||
LibC.pthread_sigmask(LibC::SIG_SETMASK, pointerof(oldmask), nil) | |||
raise Errno.new("fork", errno) | |||
raise RuntimeError.from_errno("fork", errno) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be kind of nice if Process.new
on a non-existing file raised File::NotFoundError
, i.e. all file-related errors were handled like this, even in mixed-function syscalls.
EHOSTUNREACH ENOTEMPTY EUSERS EDQUOT ESTALE EREMOTE ENOLCK ENOSYS EOVERFLOW | ||
ECANCELED EIDRM ENOMSG EILSEQ EBADMSG EMULTIHOP ENODATA ENOLINK ENOSR ENOSTR | ||
EPROTO ETIME EOPNOTSUPP ENOTRECOVERABLE EOWNERDEAD) %} | ||
{% if LibC.has_constant?(value) %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should either move this enum into Crystal::
so it is not public or have it be always the same list of errno despite the platform: this enum must have the same members regardless of platform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But not all errno values exist in all platforms. Maybe we can map those to EINVAL. But there should be at least a warning if you try to use any of those values in a platform where the value doesn't exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We decided a long time ago that the compile-time API must be identical between platforms so that everything compiles on every platform (as long as it uses documented API)
If the errno value doesn't exist on every platform, it just never gets used, never gets raised, so it can be any value.
I don't think we need warnings for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The existence of an Error number is one part of the problem. There is platform-specific value for some error codes. For example, grep -R "EDEADLK" src/lib_c/*
src/lib_c/aarch64-linux-gnu/c/errno.cr: EDEADLK = 35
src/lib_c/aarch64-linux-musl/c/errno.cr: EDEADLK = 35
src/lib_c/amd64-unknown-openbsd/c/errno.cr: EDEADLK = 11
src/lib_c/arm-linux-gnueabihf/c/errno.cr: EDEADLK = 35
src/lib_c/i386-linux-gnu/c/errno.cr: EDEADLK = 35
src/lib_c/i386-linux-musl/c/errno.cr: EDEADLK = 35
src/lib_c/i686-linux-gnu/c/errno.cr: EDEADLK = 35
src/lib_c/i686-linux-musl/c/errno.cr: EDEADLK = 35
src/lib_c/x86_64-darwin/c/errno.cr: EDEADLK = 11
src/lib_c/x86_64-dragonfly/c/errno.cr: EDEADLK = 11
src/lib_c/x86_64-freebsd/c/errno.cr: EDEADLK = 11
src/lib_c/x86_64-linux-gnu/c/errno.cr: EDEADLK = 35
src/lib_c/x86_64-linux-musl/c/errno.cr: EDEADLK = 35
src/lib_c/x86_64-macosx-darwin/c/errno.cr: EDEADLK = 11
src/lib_c/x86_64-openbsd/c/errno.cr: EDEADLK = 11
src/lib_c/x86_64-portbld-freebsd/c/errno.cr: EDEADLK = 11
src/lib_c/x86_64-unknown-freebsd/c/errno.cr: EDEADLK = 11
src/lib_c/x86_64-windows-msvc/c/errno.cr: EDEADLK = 36
io << ")" | ||
end | ||
raise Errno.new(error_message) | ||
raise RuntimeError.from_errno |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this not from_error("execvp")
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think eventually we should make an effort to improve many error messages. I'm not sure it's a good idea to leak the syscall name that failed. Maybe the same error message we have above: "Error executing process"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that could work. "execvp" has always just been a stand-in for a proper message.
Using an explicit message here would make from_errno
without arguments obsolete. I think it should be required to provide at least some message for context information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it leaking the syscall name or telling you exactly what failed, which manpage to read and what to search for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually don't think leaking the name is an issue. It's really pretty concise about defining the context of the error. (Btw. it's a libc call, not a syscall)
But in other cases, I think we can perhaps be more expressive about the error context. There's a huge problem space with localization, though. So IDK, but using just the call looks like a pretty reasonable solution, at least for the time being.
Errno was renamed in 0.34.x crystal-lang/crystal#8885 Instead we should just ignore non existent file. That way it works in 0.33 and 0.34
Errno was renamed in 0.34.x crystal-lang/crystal#8885 Instead we should just ignore non existent file. That way it works in 0.33 and 0.34
Errno was renamed in 0.34.x crystal-lang/crystal#8885 Instead we should just ignore non existent file. That way it works in 0.33 and 0.34
* Errno was replaced by RuntimeError. See crystal-lang/crystal#8885 * Update Dockerfile to use crystal 0.34.0 * AtExitHandlers was moved to crystal namespace see crystal-lang/crystal#8883 * Use new Log
The crystal 0.34.0 version includes the removal of Errno and WinError exceptions used by Halite. This last one has released a new version. crystal-lang/crystal#8885 Signed-off-by: Victor Morales <v.morales@samsung.com>
[Crystal PR #8885](crystal-lang/crystal#8885) changed the name of `IO::Timeout` to `IO::TimeoutError`
The crystal 0.34.0 version includes the removal of Errno and WinError exceptions used by Halite. This last one has released a new version. crystal-lang/crystal#8885 Signed-off-by: Victor Morales <v.morales@samsung.com>
This PR removes the usage of
Errno
andWinError
as exceptions and instead provides some new classes to distinguish different kind of errors.This is the hierarchy of the new or updated exceptions:
More subclasses might be added in the future but I think this is a good starting point.
Exception
now has an optionalos_error
containing the original error code that anyone can use in case the exception class hierarchy is not enough for proper error handlingIO::Timeout
is renamed toIO::TimeoutError
Errno
andWinerror
are now enumsException.from_errno
andException.from_winerror
eases creation of exceptions from OS errorsRuntimeError
is used, often when there is a system call that shouldn't fail in normal conditions(closes #8827)