Add Crystal::System::FileHandle by RX14 · Pull Request #4912 · crystal-lang/crystal

RX14 · 2017-09-01T17:17:52Z

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.).

RX14 · 2017-09-01T17:23:24Z

src/io/console.cr

  # 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


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

RX14 · 2017-09-01T17:30:14Z

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 · 2017-09-01T20:34:15Z

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 · 2017-09-01T23:22:06Z

@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 · 2017-09-08T13:41:21Z

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

asterite · 2017-09-08T13:47:07Z

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 · 2017-09-08T14:21:49Z

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

asterite · 2017-09-08T15:52:51Z

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

RX14 · 2017-09-08T20:34:38Z

@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 · 2017-09-08T20:49:48Z

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 · 2017-09-08T22:22:53Z

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 · 2017-09-08T23:21:20Z

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 · 2017-10-01T11:04:33Z

@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.

ysbaddaden

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.

ysbaddaden · 2017-10-01T15:01:53Z

src/crystal/system/file_handle.cr

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


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.

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.

ysbaddaden · 2017-10-01T15:14:06Z

src/crystal/system/unix/file_handle.cr

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


Misplaced comment.

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 · 2017-10-01T16:42:13Z

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 · 2017-10-01T16:54:55Z

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

bew · 2017-10-01T17:49:12Z

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 · 2017-11-30T17:37:23Z

Superseeded by #5333

Add Crystal::System::FileHandle

8589742

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

RX14 commented Sep 1, 2017

View reviewed changes

RX14 added kind:refactor pr:wip topic:stdlib labels Sep 1, 2017

ysbaddaden reviewed Oct 1, 2017

View reviewed changes

RX14 closed this Nov 30, 2017

Uh oh!

Conversation

RX14 commented Sep 1, 2017

Uh oh!

RX14 Sep 1, 2017

Choose a reason for hiding this comment

Uh oh!

RX14 commented Sep 1, 2017

Uh oh!

txe commented Sep 1, 2017

Uh oh!

RX14 commented Sep 1, 2017

Uh oh!

txe commented Sep 8, 2017

Uh oh!

asterite commented Sep 8, 2017

Uh oh!

RX14 commented Sep 8, 2017

Uh oh!

asterite commented Sep 8, 2017

Uh oh!

RX14 commented Sep 8, 2017

Uh oh!

asterite commented Sep 8, 2017

Uh oh!

RX14 commented Sep 8, 2017

Uh oh!

asterite commented Sep 8, 2017

Uh oh!

RX14 commented Oct 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ysbaddaden left a comment

Choose a reason for hiding this comment

Uh oh!

ysbaddaden Oct 1, 2017

Choose a reason for hiding this comment

Uh oh!

RX14 Oct 1, 2017

Choose a reason for hiding this comment

Uh oh!

ysbaddaden Oct 1, 2017

Choose a reason for hiding this comment

Uh oh!

RX14 Oct 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RX14 commented Oct 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RX14 commented Oct 1, 2017

Uh oh!

bew commented Oct 1, 2017

Uh oh!

RX14 commented Nov 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

RX14 commented Oct 1, 2017 •

edited

Loading

RX14 Oct 1, 2017 •

edited

Loading

RX14 commented Oct 1, 2017 •

edited

Loading