Provide supporting classes to wrap existing TCPServer and UDPSocket instances

[ioquatix]

As you asked for here https://groups.google.com/forum/#!topic/celluloid-ruby/zDG87hLmNyQ

This is a general outline. Looking for feedback before investing any more time into this.

[tarcieri]

This should work already, no?

Celluloid::IO::TCPSocket.new(ruby_tcpsocket)

Could the same be done for the other classes?

[ioquatix]

@tarcieri yes it could be, as I said it's just a general outline. Overloading constructors is okay but it also makes things a bit messy, if I know explicitly what I want, I'd rather just name it. But, ::TCPServer constructor obviously can't accept another TCPServer instance by default, so you'd be changing that behaviour/assumption.

[ioquatix]

Not sure if this is a goer or not, perhaps we need to discuss more /cc @tarcieri @digitalextremist @e2

[e2]

@ioquatix - if I'm getting this right, you want to use Celluloid and Ruby classes interchangeably, correct?

This includes constructors. What I think the conflict here is - that you want to use the Celluloid classes as both "wrappers" (by design - add Celluloid functionality) and as non-wrappers (as sockets, etc.).

And the constructors are the problem, because if you extend a constructor, it no longer matches the underlying object (socket) - and I believe that's what you're trying to avoid.

As an analogy, I think it's like the Fixnum/Bignum problem. The Fixnum/Bignum is solved through promoting one type to the other in a transparent way (as opposed to having a Bignum-like "wrapper" for Fixnum or using inheritance). I'm not sure if that's possible here, or if it make sense.

When you're doing arithmetic, you don't care whether something is a bignum or fixnum (Socket or C:Socket), and if the calculations are out of bounds (by analogy - using Celluloid-specific features), you get everything promoted to the common denominator.

Just thinking out loud here.

[ioquatix]

@e2 Practically speaking, it's used here

https://github.com/ioquatix/rubydns/blob/5f27bf2e3d444772f80c512219b759891aa61c31/lib/rubydns/server.rb#L147-L173

It allows users to provide already connected sockets for DNS server functionality.

if I'm getting this right, you want to use Celluloid and Ruby classes interchangeably, correct?

Yes, I guess at a certain level this is correct.

This includes constructors. What I think the conflict here is - that you want to use the Celluloid classes as both "wrappers" (by design - add Celluloid functionality) and as non-wrappers (as sockets, etc.).

Actually I don't really care about how it works, just that it's possible and in this case I've preferred an explicit approach.

And the constructors are the problem, because if you extend a constructor, it no longer matches the underlying object (socket) - and I believe that's what you're trying to avoid.

I guess you mean if you change Celluloid::IO::TCPServer to accept an existing ::TCPServer argument? It's possible but it's a little bit too much magic IMHO.

As an analogy, I think it's like the Fixnum/Bignum problem. The Fixnum/Bignum is solved through promoting one type to the other in a transparent way (as opposed to having a Bignum-like "wrapper" for Fixnum or using inheritance). I'm not sure if that's possible here, or if it make sense.

I agree this is nice, but I don't believe it's possible here, though feel free to propose something.

When you're doing arithmetic, you don't care whether something is a bignum or fixnum (Socket or C:Socket), and if the calculations are out of bounds (by analogy - using Celluloid-specific features), you get everything promoted to the common denominator.

Yeah, it would be nice if Celluloid::IO::*Socket wasn't required, but unfortunately it appears impossible to avoid.

[e2]

I think I get it.

Shouldn't C::UDPSocket simply be modified to accept a socket just like TCPSocket does here:

https://github.com/celluloid/celluloid-io/blob/master/lib/celluloid/io/tcp_socket.rb#L39

?

[ioquatix]

@e2 Yes that is one option, but I opted not to monkey patch the existing API in RubyDNS. I guess IMHO it isn't entirely obvious that you could/can/will be able to construct a C::IO::UDPSocket with a UDPSocket as an argument. If this is some kind of convention we want to establish for wrappers, sure, lets do that instead, but in any case, this PR is one option. It makes the operation very explicit (i.e. I want to wrap this socket).

[ioquatix]

BTW, this PR does not preclude also wrapping existing sockets.. they are not mutually exclusive.

[e2]

I guess IMHO it isn't entirely obvious that you could/can/will be able to construct a C::IO::UDPSocket with a UDPSocket as an argument.

Since C::IO::TCPSocket does this, I don't know why C::IO::UDPSocket shouldn't. Probably UDP isn't used that often, so no one noticed that C::IO::TCPSocket provides this while C::IO::UDPSocket doesn't.

C::IO::UDPSocket just does this: https://github.com/celluloid/celluloid-io/blob/master/lib/celluloid/io/udp_socket.rb#L9

Wrapping the TCPServer is a different thing, though - I agree it would be quite magical (though it should probably mirror how the sockets are wrapped).

[ioquatix]

Wrapping the TCPServer is a different thing, though - I agree it would be quite magical (though it should probably mirror how the sockets are wrapped).

My use case wrapping TCPServer and UDPSocket are the same - handling incoming connections for a server. If you do one it wouldn't make sense not to do the other.

[tarcieri]

Overloading constructors is okay but it also makes things a bit messy

Then perhaps e.g. Celluloid::IO::TCPSocket.from_io(...) ala before was the way to go.

I don't remember exactly why I removed those APIs.

[ioquatix]

Perhaps because in some cases the constructor was made to handle it?

[Asmod4n]

What should work is to create class methods and let them just call new ?

[e2]

@ioquatix - I believe you're right, because the only way to implement a factory method like .from_io is ... to pass the resource to the constructor, anyway (once you do, the factory method becomes superfluous).

So there pretty much has to be a parameter to pass it through - whether that's the first parameter (and using is_a?) or through an optional parameter (maybe cleaner, but the first parameter won't be used).

[tarcieri]

@e2 well, if we do consider it a wrapper around a single object and don't mind resorting to hax, we could do something like this:

def self.from_io(io)
  obj = allocate
  obj.instance_variable_set(:@socket, io)
  obj
end

Granted that's gross. So is the existing constructor for Celluloid::IO::TCPSocket though shrug

[e2]

@tarcieri - I had the same idea. Probably wouldn't be as gross if it was part of a "generic" way to wrap shared/reused resources.

Besides that, ::TCPSocket is technically a wrapper anyway. So it's either wrapping the wrapper, or hacking the wrapper or ... a new wrapper (and letting duck typing take care of things).

I used to appreciate strong type safety (C++ background), but I find inheritance hurts in Ruby more often than not.

Celluloid::IO::TCPSocket is basically a stream with a reference to a socket. If a socket is passed to the constructor, the distinction between TCP and UDP stops being important (in terms of API).

So the real "problem" seems to be: how to elegantly create a stream-compatible object given an existing socket/resource. Maybe Stream.from_io would be a better match? (Counter-intuitive considering inheritance, but ...)

[ioquatix]

That might be a good idea, and if you try to wrap an existing celluloid io socket it's a nop..

[ioquatix]

Perhaps a fuction Celluloid::IO::from_io(...)

[tarcieri]

@ioquatix if you had a generic wrapper like that (also, please, Celluloid::IO.from_io, this isn't 2004 :wink:), you'd lose all access to the methods on the subclasses. But perhaps it'd be a generic catch-all for all IO objects useful for people with no other recourse.

[tarcieri]

Also, I'd prefer what @e2 was suggesting: Celluloid::IO::Stream.from_io(io)

[ioquatix]

Is a TCPServer or UDPSocket a stream??

Sent from my phone.

On 24/04/2015, at 1:37 pm, Tony Arcieri notifications@github.com wrote:

Also, I'd prefer what @e2 was suggesting: Celluloid::IO::Stream.from_io(io)

― Reply to this email directly or view it on GitHub.

[tarcieri]

No, however, what kind of useful wrapper can Celluloid provide generically for those? They have method protocols specific to their class.

[ioquatix]

Well, this PR includes them..

[tarcieri]

You used subclasses, which work because you inherit all of the methods from the parent classes.

Celluloid::IO.from_io() can't work generically. At best, it could be an additional layer of complexity on top of something like this PR, but I don't think that makes sense.

Celluloid::IO::Stream.from_io(), on the other hand, can actually be generic.

[tarcieri]

Your PR is interesting, but probably backwards. The parent class should probably be the "wrapper" with a simple constructor and all the functionality, and the subclass should reimplement the constructor with more complicated Ruby-errata behavior then call super.

[ioquatix]

Forgive me but I don't see how that would work.

[ioquatix]

Oh, do you mean completely reengineering celluloids socket wrappers?

[tarcieri]

Yes, move all of the existing functionality to a wrapper class which has a simple initializer that only sets an instance variable, and having the complex, Ruby IO compatible initializer functionality in a subclass.

That way, you don't need to couple through instance variables. You can just call super with the object to be wrapped.

[hannesg]

Hi

I'm running into this issue right now, too. I need a simple way to convert ruby TCPServer/TCPSocket/UDPSocket/... into the corresponding Celluloid::IO classes. Can I help resolving this?

[ioquatix]

As @tarcieri said, I think that having a top level wrapper class might make sense. However, as proposed, my solution does work fine.

[ioquatix]

I'm going to close this in favour of the PR given by @hannesg.

[digitalextremist]

:+1: