NoMethodError: undefined method `peeraddr' for nil:NilClass

[perlun]

Hi,

Trying to get the remote host of a websocket connection fails like this, with JRuby 1.7.2:

NoMethodError: undefined method `peeraddr' for nil:NilClass
        /usr/local/appfactory-2013.6/src/server/gems/jruby/1.9/gems/reel-0.3.0/lib/reel/mixins.rb:19:in `remote_host'
        /usr/local/appfactory-2013.6/src/server/workers/relay_coordinator.rb:98:in `on_connection'
        org/jruby/RubyMethod.java:134:in `call'
        org/jruby/RubyProc.java:249:in `call'
        /usr/local/appfactory-2013.6/src/server/gems/jruby/1.9/gems/reel-0.3.0/lib/reel/server.rb:27:in `handle_connection'
        org/jruby/RubyBasicObject.java:1686:in `__send__'
        org/jruby/RubyKernel.java:1809:in `public_send'
        /usr/local/appfactory-2013.6/src/server/gems/jruby/1.9/gems/celluloid-0.13.0/lib/celluloid/calls.rb:11:in `dispatch'
        /usr/local/appfactory-2013.6/src/server/gems/jruby/1.9/gems/celluloid-0.13.0/lib/celluloid/calls.rb:96:in `dispatch'
        /usr/local/appfactory-2013.6/src/server/gems/jruby/1.9/gems/celluloid-0.13.0/lib/celluloid/actor.rb:326:in `handle_message'
        /usr/local/appfactory-2013.6/src/server/gems/jruby/1.9/gems/celluloid-0.13.0/lib/celluloid/tasks/task_fiber.rb:28:in `initialize'

[halorgium]

@perlun could you gist the code? It might be a misunderstanding which we can address.

[tarcieri]

It's breaking here:

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

This is some of the code @slivu refactored that's ostensibly breaking. Perhaps he can provide some insight. Otherwise I can track it down later.

[halorgium]

It is more a question of why the @socket is nil at this point.

[tarcieri]

@halorgium confirm

[perlun]

FWIW, my code is basically this:

class RelayCoordinator < Reel::Server
  def initialize
    port = 50000
    super(port, nil, &method(:on_connection))
  end

  def on_connection(connection)
    while request = connection.request
      case request
      when Reel::WebSocket
        puts connection.remote_host # raises exception
    end
  end
end

[tarcieri]

That should definitely work. I'm guessing some of the refactoring broke this and it wasn't covered by the tests

[halorgium]

I am noticing that you are passing the args to initialize in a way which seems to be wrong. You should pass a host (perhaps 0.0.0.0) and port. No need for the backlog.

def initialize(host, port, backlog = DEFAULT_BACKLOG, &callback)

[ghost]

@halorgium, i had to read all comments before starting investigate, this could save me about 20 mins :) @perlun, as @halorgium stated, initialize expects first argument to be the host rather that port.

@tarcieri, the only modification in regard to remote_host was to move it from remote_connection.rb https://github.com/celluloid/reel/blob/1ebf1392953ca37733ff5bf3349f85b65537aaee/lib/reel/remote_connection.rb to mixins.rb https://github.com/celluloid/reel/blob/master/lib/reel/mixins.rb#L20

[perlun]

Thanks. Should look into this, it's probably a simple usage error like you suggest.

[halorgium]

@perlun please reopen if you're still having issues.

[perlun]

I tested your approach (changing the way I call the Reel::Server constructor). However, it didn't work and I think I've found the issue...

(Couldn't reopen the issue since I'm not an admin of the Reel project, so please reopen it for me. Thanks.)

    # Read a request object from the connection
    def request
      return if @request_state == :websocket
      req = Request.read(self)

      case req
      when Request
        @request_state = :body
        @keepalive = false if req[CONNECTION] == CLOSE || req.version == HTTP_VERSION_1_0
      when WebSocket
        @request_state = @response_state = :websocket
        @socket = nil # this is the offender
      else raise "unexpected request type: #{req.class}"
      end

Hence, when I handle the WebSocket request, @socket is nil and I have no way to get the address of the remote peer. :(

Any suggestions for how to work around this? I presume @socket should be nil in these cases for some good reason...

[tarcieri]

Copypasta from #66:

This was intended to work similarly to Rack hijack: once upgraded to a WebSocket, the WebSocket is in control of the socket, not the connection. The connection should not be used after that, and setting socket to nil was flagging that fact.

I agree the semantics could be better here. I'm not sure I like your fix. I think the WebSocket should be the only object you're interacting with after the connection has been upgraded.

Undefined method peeraddr for nil is a terrible error. That said, you should probably be calling peeraddr on the WebSocket.

[ghost]

@tarcieri, sure, calling peeraddr on WebSocket works perfect, as well as remote_host and remote_ip, i tried that. I'm just trying to reduce the confusion on when to use connection and when request. Using connection on all cases could be more intuitive.

@perlun, in your context request.remote_host is working well:

    while request = connection.request
      case request
      when Reel::WebSocket
        puts request.remote_host

[tarcieri]

@silvu what I think I'd really like to do is make the WebSocket upgrade explicit by calling a method, e.g. Reel::Connection#upgrade. After this Connection should, in my opinion, be unusable as it's now been upgraded.

There are several reasons to prefer this model, for example Connection is tasked with keeping track of things like the request/response cycle and HTTP pipelining. When the connection is upgraded to a websocket it becomes asynchronous and none of these things are possible.

This is why I want to push people to use the WebSocket object exclusively when dealing with WebSockets.

[ghost]

makes sense. how about to find a elegant way to raise a relevant exception when ppl trying to use @socket on websocket requests.

[tarcieri]

That'd be great! It should definitely raise an exception informing people that the connection has already been upgraded to a WebSocket

[halorgium]

class SocketRaiser
  %w{ foo bar }.each do |meth|
    class_eval <<-EOT
      def #{meth}(*args)
        raise "No socket for you to invoke the `#{meth.inspect}' method"
      end
    EOT
  end
end

[tarcieri]

This is now handled in a single place by Reel::Connection#socket as of the changes in #77