celluloid / celluloid-io

UNMAINTAINED: See celluloid/celluloid#779 - Evented sockets for Celluloid actors
https://celluloid.io
MIT License
879 stars 93 forks source link

Use a common super class for all socket wrappers #160

Closed hannesg closed 8 years ago

hannesg commented 8 years ago

This is my approach on the proposed solution for #129

The new superclass is called Celluloid::IO::Socket and essentialy deduplicates all #to_io methods and many delegates. This allows all socket wrappers to be created from a raw ruby socket using their default initializer or the new C::IO::Socket.try_convert method.

I've added some methods for backward compatibility so that code like this doesn't break:

class MyClass
  include Celluloid::IO
  def my_method
    Socket.new(Socket::AF_INET, Socket::SOCK_STREAM, 0)
  end
end

Cheers' Hannes

ioquatix commented 8 years ago

This looks really good. I'll help where I can with merging and testing. This PR introduces a few new things into the Celluloid namespace. Is there any chance this will break existing code?

Can we also review the errors on Travis?

tarcieri commented 8 years ago

Hmm, interesting. In general I think it looks good but I might try to leave you some line notes at some point.

ioquatix commented 8 years ago

@hannesg Can you please address my feedback and then let's get this merged. It's a good idea, the implementation looks fine, I'd like to test it.

digitalextremist commented 8 years ago

The way the try_convert method handles individual socket types in a static switch/case statement is going to bottleneck/undermine the intention of this change.

I'd reference the interface to Celluloid::SystemEvent which allows dynamic handling of different types of object, via the handler block definition at a superclass level, then invocation of that on a subclass level... in this case that'd be converter or the like:

https://github.com/celluloid/celluloid/blob/master/lib/celluloid/system_events.rb#L20

ioquatix commented 8 years ago

@digitalextremist That handler method looks, on the surface, incredibly inefficient, doing all that string manipulation.

ioquatix commented 8 years ago

I'm going to merge this and then fix up any remaining issues.

ioquatix commented 8 years ago

Okay this is now merged and normalised. Thanks so much for your contributions.