celluloid / celluloid-io

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

Celluloid::IO 0.15.0 incompatible with Net::SSH 2.8.x #99

Closed HoneyryderChuck closed 9 years ago

HoneyryderChuck commented 10 years ago

Just tried using Net::SSH connection with Celluloid::IO::TCPSocket using the latest versions and it failed. It seems that it passes an hash to the third argument of the TCPSocket constructor, which is expected to be a String by Celluloid::IO.

I downgraded to version 2.0.14 of Net::SSH and everything worked.

tarcieri commented 10 years ago

We should try to document the issue around the missing third parameter. That's functionality we should support.

It's an options hash? I don't have time ATM to look into it more specifically, maybe tonight.

HoneyryderChuck commented 10 years ago

Here is the exact error (using net-ssh 2.8.0):

Net::SSH.start('localhost', 'localuser') # all good
Net::SSH.start('localhost', 'localuser', proxy: Celluloid::IO::TCPSocket)
TypeError: can't convert Hash into String 
from /gems/celluloid-io-0.15.0/lib/celluloid/io/tcp_socket.rb:82:in `tcp'

Problem seems to be the third argument, local_host, whose value is an hash using net-ssh. And the strangest is, not even the raw TCPSocket initialize method handles hashes as third argument. But somehow connections are established anyways using net-ssh with raw tcpsockets.

HoneyryderChuck commented 10 years ago

I just confirmed, the latest compatible version is 2.7.0 . Don't know what changed exactly.

tarcieri commented 10 years ago

Is it passing in a bogus argument that MRI tolerates for some wacky reason?

Any chance you could bisect?

Asmod4n commented 10 years ago

Maybe just copy what Rubinius is doing https://github.com/rubysl/rubysl-socket/blob/2.0/lib/rubysl/socket.rb

Von einem mobilen Gerät gesendet

Am 18.03.2014 um 03:49 schrieb Tony Arcieri notifications@github.com:

Is it passing in a bogus argument that MRI tolerates for some wacky reason?

Any chance you could bisect?

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

HoneyryderChuck commented 10 years ago

Yup, I fail to see the wacky reason they are tolerating it. The third argument seems to be a Net::SSH options list (I remember I saw the auth_methods list there).

HoneyryderChuck commented 9 years ago

I guess this has to do with net-ssh and how it handles proxies, celluloid doesn't have anything to do wiht it.

HoneyryderChuck commented 9 years ago

The "issue" has been clarified. See https://github.com/net-ssh/net-ssh/issues/220#issuecomment-70456647

I think this is more a question of misleading documentation, see https://github.com/celluloid/celluloid-io/wiki/Libraries-that-support-Celluloid%3A%3AIO for net-ssh compatibility. You cannot unfortunately just override the proxy class, but provide a "proxified" way to initialize the socket.

Would like to see the proxy feature also documented on the net-ssh side, though.

digitalextremist commented 9 years ago

@TiagoCardoso1983, I linked your comment to the Wiki. Feel free to expound and I will port your caveats to the Wiki, then reopen this ticket and ping me.