Seperate Socket and FileDescriptor IO #4707

RX14 · 2017-07-12T21:54:57Z

On some platforms - notably windows - socket descriptors are different from file
descriptors so it makes little sense for them to be shared under a common hierarchy.

This is done in 3 stages:

Firstly I refactored out the common logic for dealing with nonblocking IO using read/write like functions from IO::FileDescriptor into IO::Syscall. It takes care of checking for EAGAIN and managing the lists of waiting fibers.
Then, I removed the edge_triggerable option from IO::FileDescriptor, it doesn't seem to be used anywhere and it seems to me like it would cause hangs and other weird behaviour if used.
Finally, I changed Socket not to inherit IO::FileDescriptor. This involved quite a lot of copy-pasting code, which seems like a step backwards, but it means we can refactor File and Socket to use Crystal::System separately. Once these refactors are done both File and Socket class hierarchies will likely end up much cleaner.

RX14 · 2017-07-12T22:04:24Z

src/socket.cr

-    # see IO::FileDescriptor#unbuffered_write
-    if (writers = @writers) && !writers.empty?
-      add_write_event
+    write_syscall_helper(slice, "Error sending message") do |slice|


I'm not very sure this behaviour is correct, as write_syscall_helper retries send. This looks correct for stream-type sockets but not for datagram-type sockets, but send/recv are used for both. However, datagram-type sockets still implement IO so i'm not sure what to do. Socket in general feels like a messy part of the stdlib to me.

It musn't retry. Send with either succeed to send the whole messages, or fail and raise an exception.

I realise that it's way beyond the scope of this PR but I really think we should have seperate types for datagram and stream-type sockets. The send syscall is basically overloaded to do 2 different things: copy data to the kernel's output buffer for stream sockets and send a message for datagram sockets. I think it's a mistake to confuse these by putting it all under one type.

Fixed this by simply not touching the "message" parts of Socket, and the diff is a lot cleaner as a result.

ysbaddaden · 2017-07-12T22:55:56Z

I hardly see a benefit in this PR for the time being (e.g. code duplication), and how it will simplify integrating WinSock (which are BSD sockets, thus use file descriptors, unlike windows files).

Calls to read and write (generic fd calls) are also kept in Socket, not the abstraction... but they're impossible on windows: there are no generic fds, only sockets so it has special calls (AFAIK).

RX14 · 2017-07-12T23:07:50Z

@ysbaddaden The point is so that Socket no longer has any dependency on read and write, because winsock uses only send and recv. It also allows you to refactor File and Socket IO seperately. This PR is a result of me refactoring IO::FileDescriptor to use Crystal::System which maintaining Socket < IO::FileDescriptor. The resulting PR is really really messy on the Socket side so I thought i'd PR this first. You can see that branch here if you want to see my rationale.

sempervictus · 2017-07-13T06:07:05Z

There's a Ruby library called rex-socket which deals with some of the concerns here. It's effectively a set of abstractions we use for dealing with remote networking requirements and unconventional activities (a core part of Metasploit). The approach uses Ruby modules to define common functionality and apply in class composition via mixins (sometimes via extend because we pull from Ruby's stdlib c primitives). This really helps reduce redundancy as only the differentiated parts are written in the class itself. Over time creep will occur, but beats copy pasta everywhere and provides consistent interfaces and coding convention for a critical and touchy part of any language.
Gram, nix, netlink, etc sockets are very different from stream socks like TCP. While users may want convenience around them to make them feel familiar, that sort of thing belongs outside a standard library (IMHO).
Windows handles are definitely an oddity, so if multi platform support is the intent, its probably best to disambiguate the difference for developers. Nobody wants a repeat of Ruby on Windows :).

RX14 · 2017-07-13T20:45:00Z

Looks to me like Travis now doesn't have enough memory to reliably compile all_spec. There have been quite a few failed builds due to fork: Cannot allocate memory recently (even on master).

akzhan · 2017-07-13T21:26:09Z

What prevents to run specs separately through find | xargs bin/crystal? With -P4 for CircleCI.

RX14 · 2017-07-13T22:12:09Z

@akzhan the spec output will be almost too ugly to read and it'll be a lot less efficient in terms of compilation time. ~~Probably offset by having multiple cores available though~~.

time make spec reports 9 minutes and 23 seconds on my quad core i5 desktop. find spec/compiler spec/std -type f -name '*_spec.cr' | time parallel env CRYSTAL_CACHE_DIR=/tmp/crystal_cache_{%} bin/crystal spec {} reports 28 minutes 53 seconds. Clearly this isn't a good idea, the extra compilation work doesn't even nearly benefit from using all of 4 cores.

funny-falcon · 2017-07-16T00:09:05Z

may be set --threads 1 option for compiler?

funny-falcon · 2017-07-16T00:35:23Z

looks like --threads 1 helped. See #4675

bcardiff · 2017-07-30T21:00:39Z

Regarding the edge_triggerable. It was added by a0820c7 to fix something in darwin but it's usage seems to got lost at fe3ccc5 maybe the timeouts work as an alternative solution for the issue in darwin #706 .

I think is good to remove the inheritance between Socket and FileDescriptor, but I would feel more comfortable knowing what is the implementation needed for windows in order to judge if the abstraction is appropriate.

RX14 · 2017-07-30T21:26:47Z

Memory issues on travis also need to be resolved before this can be merged and I have no idea how to proceed on that.

@bcardiff maybe I should work further into the future and attempt to port windows on top of this PR + abstraction IO to Crystal::System. The danger however is that this PR gets rejected and all that work is wasted. Thoughts?

sdogruyol · 2017-08-16T09:11:50Z

Seems like CircleCI is happy. LGTM?

mverzilli · 2017-08-19T23:15:28Z

@RX14 can you resync with master now that Ary split the CI runnables? Let's move this forward. I wouldn't be too shy about finding the perfect abstraction: on the way to getting Windows support in master we're likely to break things loudly many times :D.

RX14 · 2017-08-20T08:05:39Z

Yup, if this turns out to be wrong I'm happy to do the work to put them all under the same hierarchy again. I'll rebase this and recheck my work in a bit.

RX14 · 2017-08-20T16:41:10Z

This isn't ready to merge just yet.

I also hit this while refactoring, is it a bug?

    def add(timeout : LibC::Timeval? = nil)
      if timeout
        typeof(timeout) # => LibC::Timeval
        LibEvent2.event_add(@event, pointerof(timeout))
      else
        LibEvent2.event_add(@event, nil)
      end
    end

# argument 'timeout' of 'LibEvent2#event_add' must be Pointer(LibC::Timeval), not Pointer(Nil)

# doesn't make sense

    def add(timeout : LibC::Timeval? = nil)
      if timeout_copy = timeout
        LibEvent2.event_add(@event, pointerof(timeout_copy))
      else
        LibEvent2.event_add(@event, nil)
      end
    end

# even making a copy doesn't work

asterite · 2017-08-20T16:56:31Z

@RX14 The problem is that timeout's type is a nilable type, an pointerof of a nilable type is not what you want. You can assign timeout to a different variable inside if timeout and then take a pointer to that, maybe that'll work.

RX14 · 2017-08-20T16:57:18Z

@asterite that's exactly what my second example does with timeout_copy.

asterite · 2017-08-20T16:58:34Z

I mean:

if timeout
  timeout_copy = timeout
end

In if timeout_copy = timeout, timeout_copy is still nilable (you are assigning exactly timeout to it, before the type is filtered)

RX14 · 2017-08-20T16:59:19Z

@asterite Well if you use if foo = @foo where @foo is nillable, foo is never nillable.

asterite · 2017-08-20T16:59:57Z

@RX14 I know, but the compiler doesn't :-P

asterite · 2017-08-20T17:05:11Z

@RX14 Actually, no:

if foo = @foo
end

# but here foo can be nilable

The only place where foo is not nilable is inside the if.

RX14 · 2017-08-20T17:09:37Z

Aargh but it feels wrong! I never think of if foo = <expr> as having foo scoped outside the body of the if. It's better than defining def add(timeout : Nil = nil) at least.

IO::Syscall is an abstraction over non-blocking IO provided by syscalls. It takes care of checking for EAGAIN and managing the lists of waiting fibers.

On some platforms - notably windows - socket descriptors are different from file descriptors so it makes no sense for them to be shared under a common hierarchy.

RX14 · 2017-08-20T17:25:09Z

I've refactored this a bit to store a Time::Span in IO::Syscall not a LibC::Timeval. This is much cleaner but has the downside of converting each time we call libevent. I think it's worth it though.

sdogruyol · 2017-08-22T13:03:02Z

Let's approve this 👍

RX14 · 2017-08-29T11:42:24Z

Still no second review? :(

akzhan · 2017-08-29T12:09:23Z

I'm unsure that conversions on every libevent call is OK. Any benchmarks?

mverzilli · 2017-08-30T02:31:13Z

src/io/file_descriptor.cr

-      add_read_event
+    read_syscall_helper(slice, "Error reading file") do
+      # `to_i32` is acceptable because `Slice#size` is a Int32
+      LibC.read(@fd, slice, slice.size).to_i32


Why do we need this explicit conversion?

Because read_syscall_helper is explicitly annotated to return i32.

Yeah, but read_syscall_helper didn't exist before this refactor, so why does it need to be explicitly annotated?

Because IO#read should always return typeof(Bytes.new.size). That wasn't the case before.

Got it :). Thanks for bearing with me!

It would be fantastic if we could force return types from an abstract declaration. Returning anything other than i32 from read doesn't make sense, and a return value from write is similarly useless. But that's (literally) another issue.

Yeah, we were discussing exactly that with @matiasgarciaisaia in relation to #4864).

RX14 · 2017-08-30T14:41:17Z

@akzhan Actually, every call to libevent will have been immediately preceded by a context switch (EAGAIN). Adding 8 or so cycles to that I doubt will affect performance.

RX14 · 2017-08-30T14:53:02Z

Reminder for the changelog that this is a breaking change.

mverzilli · 2017-09-03T15:18:06Z

Just created a breaking-change label to keep track of those in particular.

faustinoaq · 2017-09-21T07:33:33Z

@mverzilli Do you think breaking-change should be kind:breaking-change ? I mean just keeping label pattern 😅

mverzilli · 2017-09-21T12:50:49Z

It's an ortogonal dimension in my head. Let's keep it like that for now, I don't see any harm.

RX14 added kind:refactor pr:needs-review topic:stdlib labels Jul 12, 2017

RX14 commented Jul 12, 2017

View reviewed changes

RX14 force-pushed the feature/separate-socket-io branch 2 times, most recently from 3bc1658 to 6429494 Compare July 13, 2017 19:51

akzhan mentioned this pull request Jul 15, 2017

change computation of hash value. #4675

Closed

RX14 mentioned this pull request Jul 29, 2017

[WIP] Windows #3582

Closed

24 tasks

mverzilli mentioned this pull request Aug 19, 2017

Improve JSON and YAML exception locations #4855

Merged

mverzilli approved these changes Aug 19, 2017

View reviewed changes

RX14 force-pushed the feature/separate-socket-io branch from 6429494 to b93251a Compare August 20, 2017 15:48

RX14 added 3 commits August 20, 2017 18:10

Add IO::Syscall

29fbc5f

IO::Syscall is an abstraction over non-blocking IO provided by syscalls. It takes care of checking for EAGAIN and managing the lists of waiting fibers.

Remove the edge_triggerable option from IO::FileDescriptor

b153aa5

Seperate Socket and IO::FileDescriptor

98eba71

On some platforms - notably windows - socket descriptors are different from file descriptors so it makes no sense for them to be shared under a common hierarchy.

RX14 force-pushed the feature/separate-socket-io branch from b93251a to 98eba71 Compare August 20, 2017 17:14

mverzilli reviewed Aug 30, 2017

View reviewed changes

mverzilli approved these changes Aug 30, 2017

View reviewed changes

RX14 merged commit c26fbc9 into master Aug 30, 2017

RX14 added this to the Next milestone Aug 30, 2017

RX14 removed the pr:needs-review label Aug 30, 2017

RX14 mentioned this pull request Sep 1, 2017

Add Crystal::System::FileHandle #4912

Closed

mverzilli added the breaking-change label Sep 3, 2017

bcardiff mentioned this pull request Sep 29, 2017

Fix socket initialization #5059

Merged

asterite deleted the feature/separate-socket-io branch April 27, 2018 21:22

asterite mentioned this pull request Oct 3, 2018

Refactor Socket to reuse Crystal::System::FileDescriptor #6906

Closed

Seperate Socket and FileDescriptor IO #4707

Seperate Socket and FileDescriptor IO #4707

Conversation

RX14 commented Jul 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ysbaddaden commented Jul 12, 2017

RX14 commented Jul 12, 2017

sempervictus commented Jul 13, 2017

RX14 commented Jul 13, 2017

akzhan commented Jul 13, 2017

RX14 commented Jul 13, 2017 • edited Loading

funny-falcon commented Jul 16, 2017

funny-falcon commented Jul 16, 2017

bcardiff commented Jul 30, 2017

RX14 commented Jul 30, 2017

sdogruyol commented Aug 16, 2017

mverzilli commented Aug 19, 2017

RX14 commented Aug 20, 2017

RX14 commented Aug 20, 2017 • edited Loading

asterite commented Aug 20, 2017

RX14 commented Aug 20, 2017

asterite commented Aug 20, 2017 • edited Loading

RX14 commented Aug 20, 2017

asterite commented Aug 20, 2017

asterite commented Aug 20, 2017

RX14 commented Aug 20, 2017

RX14 commented Aug 20, 2017

sdogruyol commented Aug 22, 2017

RX14 commented Aug 29, 2017

akzhan commented Aug 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RX14 Aug 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RX14 commented Aug 30, 2017 • edited Loading

RX14 commented Aug 30, 2017

mverzilli commented Sep 3, 2017

faustinoaq commented Sep 21, 2017

mverzilli commented Sep 21, 2017

RX14 commented Jul 13, 2017 •

edited

Loading

RX14 commented Aug 20, 2017 •

edited

Loading

asterite commented Aug 20, 2017 •

edited

Loading

akzhan commented Aug 29, 2017 •

edited

Loading

RX14 Aug 30, 2017 •

edited

Loading

RX14 commented Aug 30, 2017 •

edited

Loading