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

Add Crystal::System::FileHandle #4912

Closed
wants to merge 1 commit into from

Conversation

RX14
Copy link
Contributor

@RX14 RX14 commented Sep 1, 2017

Crystal::System::FileHandle represents a handle to a file, or file-like
object (pipe, tty, etc.).

The specs currently fail due to debug line-numbers being broken for some weird reason. I just wanted to PR this so people could criticize the technique.

Crystal::System::FileHandle represents a handle to a file, or file-like
object (pipe, tty, etc.).
@@ -21,15 +21,15 @@ class IO::FileDescriptor
# This will prevent displaying back to the user what they enter on the terminal.
# Only call this when this IO is a TTY, such as a not redirected stdin.
def noecho!
if LibC.tcgetattr(fd, out mode) != 0
if LibC.tcgetattr(@handle.platform_specific, out mode) != 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A lot of this stuff still needs to be platform abstracted.

@RX14
Copy link
Contributor Author

RX14 commented Sep 1, 2017

One thing I'm concerned about is that we've separated Socket and FileDescriptor but we still have pipes and consoles and similar things. I think it's pretty unreasonable to have a seperate hierarchy of code under IO for each type but I want to remove FileDescriptor too and leave just Socket < IO, File < IO, etc.

@txe
Copy link
Contributor

txe commented Sep 1, 2017

It's unclear for me about Socket < IO, now it's just

class Socket
    include IO::Buffered
    include IO::Syscall
....
end

So, what is the purpose of making Socket < IO?

@RX14
Copy link
Contributor Author

RX14 commented Sep 1, 2017

@txe By that comment I simply meant that Socket would be an IO directly, instead of inheriting from IO::FileDescriptor. That work has already been done, but I thought about it a bit more and removing IO::FileDescriptor and being able to do the same to File would be great. The unfortunate part is that there are a lot of IO objects in unix systems which are neither files or sockets; they're pipes or ttys or character devices or block devices. Separating all these types into different classes is unreasonable. So I'm questioning the design decisions behind this PR and #4707 a little bit.

@txe
Copy link
Contributor

txe commented Sep 8, 2017

@RX14 It would be great if you could finish this work. It will really help me with adding functionality for windows.

@asterite
Copy link
Member

asterite commented Sep 8, 2017

Maybe IO::FileDescriptor can be kept for unix, and for Windows we can have a different hierarchy. The only thing IO::FileDescriptor exposes is fd, which shouldn't be normally used.

@RX14
Copy link
Contributor Author

RX14 commented Sep 8, 2017

I don't think that changing the class hierarchy based on platform will turn out well...

@asterite
Copy link
Member

asterite commented Sep 8, 2017

It could be an included module, just an implementation detail.

@RX14
Copy link
Contributor Author

RX14 commented Sep 8, 2017

@asterite Except my concern is that we have to have file descriptors which are neither sockets or files. Modules can't be instantiated by their own so that doesn't solve much.

@asterite
Copy link
Member

asterite commented Sep 8, 2017

I mean, File, Socket and pipes could include IO::FileDescriptor. Or Socket in Windows doesn't inherit IO::FileDescriptor. In any case I don't think a different hierarchy is a problem, with an IO you want to read and write.

@RX14
Copy link
Contributor Author

RX14 commented Sep 8, 2017

Well most of the the role of IO::FileDescriptor was surplanted in this PR, which is why I wanted to remove IO::FileDescriptor. Are you suggesting having a seperate class for pipes? What about device nodes and character devices and all the other weird and wonderful uses of file descriptors? I'm not entirely sure I understand your proposed design.

@asterite
Copy link
Member

asterite commented Sep 8, 2017

  • unix: IO::FileDescriptor exists for files, sockets, pipes, etc.
  • windows: IO::FileDescriptor exists for files, pipes, etc. Sockets have a different implementation.

That's what I'm saying.

@RX14
Copy link
Contributor Author

RX14 commented Oct 1, 2017

@asterite The problem with that proposal is that it means that Socket will have to be split into 2 platform specific files. So much code will change in the Socket class that it would be impossible to follow if the platform-specifics were done using macros. I don't think changing inheritance based on platform is even practical without splitting the files entirely.

I think this PR is probably the best we can do for now.

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.

I can't say whether this change is good or not. There are no Windows example to review and balance the changes and decide.

IMHO IO::FileDescriptor is left spineless. It delegates everything to Crystal::System::FileHandle and does nothing. Why bother keeping it?

Why the Crystal::System::FileHandle naming? It's only a handle in 1 case (Windows) and a descriptor for all other targets. It's not even a file, neither a file handle on Windows.

I think I'd go with @asterite change to make IO a class, first, then have a Crystal::System::IO module and IO::System class:

module Crystal::System::IO
  # actual impl
end

class IO::System < IO
  # documented, empty, methods

  include Crystal::System::IO
end

class File < IO::System; end
class Socket < IO::System; end

I would also have system specific namings (i.e. fd or handle) and a generic #to_unsafe to access either one.

# Returns the platform-specific representation of this handle.
#
# On unix-like platforms this returns an Int32 file descriptor.
# def platform_specific
Copy link
Contributor

Choose a reason for hiding this comment

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

platform_specific is the worst possible naming. If it's supposed to represent the underlying system data, then maybe raw or even handle if we go with windows naming (bad idea: Windows is one target, POSIX is all others) or just a plain #to_unsafe so we can pass @handle directly to C functions instead of the horrifying @handle.platform_specific we see everywhere.

In fact, I don't even see any reason to abstract the naming here: any POSIX system specifics will expect a file descriptor, and Windows system specifics will expect a HANDLE.

Trying to abstrtact this is actually confusing and misleading. We could have the false impression that LibC.tcsetattr(@handle.platform_specific, ...) will just work on Windows for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you're right that it's bad naming, but I don't think that #to_unsafe is a good way to handle this. It's way too magic to pass a crystal object and it magically gets converted into a file descriptor. I think automatically calling to_unsafe is way too magic in general...

I think you're probably right about naming it fd on posix and handle on windows.

raise Errno.new("Could not reopen file descriptor")
end
{% else %}
# dup doesn't copy the CLOEXEC flag, copy it manually to the new
Copy link
Contributor

Choose a reason for hiding this comment

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

Misplaced comment.

Copy link
Contributor Author

@RX14 RX14 Oct 1, 2017

Choose a reason for hiding this comment

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

I'm confused, it's missing a word but why is it misplaced? It's documenting the implementation so it should be inside the method.

@RX14
Copy link
Contributor Author

RX14 commented Oct 1, 2017

IMHO IO::FileDescriptor is left spineless. It delegates everything to Crystal::System::FileHandle and does nothing. Why bother keeping it?

I'm not sure we should be exposing Crystal::System to the user, and there are some file descriptors which don't fit into any of the subclasses. I can't see a way to remove it.

Why the Crystal::System::FileHandle naming? It's only a handle in 1 case (Windows) and a descriptor for all other targets. It's not even a file, neither a file handle on Windows.

According to wikipedia, file descriptors are a type of handle. I agree with wikipedia's definition.

@RX14
Copy link
Contributor Author

RX14 commented Oct 1, 2017

I think we should merge #4901 before this PR though.

@bew
Copy link
Contributor

bew commented Oct 1, 2017

Still from wikipedia:

In computer programming, a handle is an abstract reference to a resource.

@RX14 I think it's important to emphasis the fact that it is a resource handler, and not simply a generic handler, or a more precise one like file handler.

I also agree on this definition of a resource handler, and how a FD, socket, PID, etc.. are also resource handlers. But in our case, I think it's a concept too generic to use in a programming language, and we should have classes for the abstraction level above, which we can actually implement using syscalls and stuff.

@RX14
Copy link
Contributor Author

RX14 commented Nov 30, 2017

Superseeded by #5333

@RX14 RX14 closed this Nov 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants