Async::WebSocket::Incoming initial implementation

[bryanp]

@ioquatix Here's the prototype for what I was proposing a few days ago. Allows WebSocket connections to be created directly from an Async::HTTP::Request:

Async.run do
  Async::HTTP::Server.for(endpoint, protocol) { |request|
    if socket = Async::WebSocket::Incoming.new(request)
      # we have an open socket
    end
  }.run
end

I intentionally left out checking whether the request is a valid WebSocket request and can be hijacked. The approach I like the most is for initialize to do this and raise an exception:

def initialize(request)
  @env = build_env(request)
  @url = build_url(request)

  if request.hijack? && websocket?(@env)
    super hijacked_io(request), ::WebSocket::Driver.rack(self)
  else
    raise InvalidConnection
  end
end

def websocket?(env)
  ::WebSocket::Driver.websocket?(env)
end

Could alternatively use the self.open approach from Async::WebSocket::Server but I prefer the explicitness of an exception rather than a more implicit nil check. In my case, I want to handle invalid connection attempts by explicitly returning a bad request--the exception makes it more clear.

# explicit exception 
#
class WebSocket
  def initialize(connection)
    @socket = Async::WebSocket::Incoming.new(connection.request)
    # ...
  rescue Async::WebSocket::InvalidConnection
    # handle
  ensure
    @socket&.close
  end
end

# nil check
#
class WebSocket
  def initialize(connection)
    if socket = Async::WebSocket::Incoming.new(connection.request)
      # ...
    else
      # handle
    end
  ensure
    socket.close
  end
end

Since this mostly comes down to preference, I'll defer to you on this.

[bryanp]

Another responsibility of Async::WebSocket::Incoming should be closing the socket. We could hold on to the socket and close after calling super, which closes the driver:

def close
  super
  @io.close
end

It would be the responsibility of the code that creating the Async::WebSocket::Incoming to close it (see my original example above). Alternatively, initialize could yield:

def initialize(request)
  @env = build_env(request)
  @url = build_url(request)
  @io = hijacked_io(request)

  super @io, ::WebSocket::Driver.rack(self)

  if block_given?
    yield self
    close
  end
end

[ioquatix]

This is a really great start.

[ioquatix]

I like what you propose here:

def initialize(request)
  @env = build_env(request)
  @url = build_url(request)
  @io = hijacked_io(request)

  super @io, ::WebSocket::Driver.rack(self)

  if block_given?
    yield self
    close
  end
end

I think you should consider whether an open method makes more sense in the context of yield self w.r.t. derived classes.

Also, consider that we should fail fast and before allocations if it cannot be a websocket connection.

[bryanp]

Excellent points. I think I prefer the open approach, except for the fact that we'll need to either:

1. Build the env hash twice (once in open and another in initialize)
2. Or, pass the env to initialize

If 2, we'd be better off reusing Async::WebSocket::Server and adding a from_request method. I've nearly talked myself into this being a better design, but still going back and forth.

Another related thought. In working with async-http I've hit a few cases where I need to build an env-like hash from the same underlying request. It makes me wonder if async-http should include a class that can wrap an Async::HTTP::Request and build/cache an env:

class Wrapped < DelegateClass(Async::HTTP::Request)
  def to_env
    @env ||= {
      ...
    }
  end
end

Alternatively, the wrapping class could be a hash-like object itself and respond to rack env lookups:

class Wrapped < DelegateClass(Async::HTTP::Request)
  def [](key)
    if key.start_with?("HTTP_")
      headers[key[5..-1].downcase]
    else
      case key
      when "rack.input"
        body
      ...
    end
  end
end

Since parsing keys takes work, a combination of both approaches might be ideal.

I find myself building partial env hashes like what you see in this PR, simply because so much of the web-related libraries for ruby are build around rack. While I don't think that Async::HTTP::Request itself should conform to the design decisions of rack, including an object that makes it easier to integrate into the current ecosystem might make sense. I realize this is probably a larger discussion, so if it's something you think async-http should consider supporting I'll create an issue there.

[ioquatix]

Why not have open return a connection or nil if it wasn't a websocket, and use it like:

if connection = Connection.open(request)
else
  # not websocket
end

[ioquatix]

class Connection
  def self.build_env(request)
    # bail out as soon as possible if not websocket
  end

  def self.open(request)
    # very rough idea
    if env = build_env(request)
      yield self.new(request, env)
    end
  end
end

[ioquatix]

I completely forgot about this and I have rebuilt this entire library.

It would make more sense to add a server handler similar to Async::WebSocket::Server::Rack but for what you want, e.g. Async::WebSocket::Server::Native (please think of a better name).

Can you rebase this PR on master?

[ioquatix]

This discussion was helpful and I believe we've addressed most of the points here. If not please feel free to reopen.