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

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

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

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

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

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

may be set --threads 1 option for compiler?

looks like --threads 1 helped. See #4675

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.

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?

Seems like CircleCI is happy. LGTM?

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

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.

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

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

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

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)

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

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

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

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.

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.

Let's approve this 👍

Still no second review? :(

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

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

Reminder for the changelog that this is a breaking change.

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

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

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