Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support connection upgrade using rack.hijack partial hijack. #2914

Closed
ioquatix opened this issue Aug 22, 2022 · 13 comments
Closed

Support connection upgrade using rack.hijack partial hijack. #2914

ioquatix opened this issue Aug 22, 2022 · 13 comments
Labels

Comments

@ioquatix
Copy link
Contributor

It would be nice if Puma can support WebSockets, even if it was limited to one WebSocket per thread. Allocation 100 or 1000 threads per instance isn't a bad model for some use cases.

I've been playing around with this, but can't get it to work.

I have a sample server:

require 'async/websocket/adapters/rack'

app = lambda do |env|
  Async::WebSocket::Adapters::Rack.open(env) do |connection|
    while message = connection.read
      connection.write message
    end
  end or [404, {}, []]
end

run app

This returns a response which looks like this:

[101,
 {"sec-websocket-extensions"=>"permessage-deflate;client_max_window_bits=15",
  "sec-websocket-accept"=>"1q4ves5M1K+fV+tvlOTSdG9WzZQ=",
  "upgrade"=>"websocket",
  "rack.hijack"=>
   #<Async::HTTP::Body::Hijack #<Proc:0x000000010849da78 /Users/samuel/Projects/socketry/async-websocket/lib/async/websocket/adapters/http.rb:53>>},
 []]

I have a proposal to support a better upgrade mechanism, but in theory, this should be sufficient. However, I get the following error:

2022-08-22 21:47:19 +1200 HTTP parse error, malformed request: #<Puma::HttpParserError: Invalid HTTP format, parsing fails. Are you trying to open an SSL connection to a non-SSL Puma?>

I'm guessing something is wrong at the protocol level. rack.hijack never seems to be called. I wonder if Puma is trying to buffer all the input? rack.hijack probably expects streaming input.

@ioquatix
Copy link
Contributor Author

I made a minimal (non-web socket) server just to see what I could trigger:

app = lambda do |env|
	p env
	callback = proc do |stream|
		binding.irb
	end
	
	[101, {'rack.hijack' => callback, 'upgrade' => 'websocket'}, []]
end

I noticed that even for an incoming connection with upgrade: web socket, env contains "rack.input"=>#<Puma::NullIO:0x0000000104bf3830> which seems... incorrect? Surely we can read from the input stream here?

@MSP-Greg
Copy link
Member

@ioquatix

Re the above, were you using a TLSv1.3 connection? Could you try it forced to TLSv1.2?

@ioquatix
Copy link
Contributor Author

@MSP-Greg I believe I was just using plain HTTP, no TLS. But I can double check.

@MSP-Greg
Copy link
Member

Could something have used port 443? I'll check the code, but that error only happens when an http connection is made to a server setup to use only SSL clients, and also with TLSv1.3 as the negotiated protocol?

@ioquatix
Copy link
Contributor Author

https://github.com/ioquatix/puma-switching-protocols

I was trying to get this to work. I can't seem to get the rack.hijack callback invoked. Did I do something wrong?

@dentarg
Copy link
Member

dentarg commented Aug 23, 2022

https://github.com/ioquatix/puma-switching-protocols

If I change the status code from 101 to 200 the callback is invoked (I don't know why) Saw it here:

def test_http_10_partial_hijack_with_content_length
body_parts = ['abc', 'de']
server_run do
hijack_lambda = proc do | io |
io.write(body_parts[0])
io.write(body_parts[1])
io.close
end
[200, {"Content-Length" => "5", 'rack.hijack' => hijack_lambda}, nil]
end
data = send_http_and_read "GET / HTTP/1.0\r\nConnection: close\r\n\r\n"
assert_equal "HTTP/1.0 200 OK\r\nContent-Length: 5\r\n\r\nabcde", data
end

@ioquatix
Copy link
Contributor Author

Oh, great, let me try it.

@ioquatix
Copy link
Contributor Author

I've created rack/rack#1954 as a proposal for how we can solve protocol upgrades with a shared semantic between HTTP/1 and HTTP/2. While we still need to do protocol specific things, actually handling the streaming is a bit easier with shared understand of "this request wants to use the following protocol, and this response will use that protocol".

@ioquatix
Copy link
Contributor Author

ioquatix commented Aug 29, 2022

HTTP/1 and HTTP/2 have distinct mechanisms supporting websocket.

HTTP/1 requires connection: upgrade and upgrade: websocket. HTTP/2 requires :method CONNECT and :protocol websocket. Both have subtle differences like request and response methods, status codes, etc.

I would like to know if you are interested in exploring a model which I've been using in async-websocket gem, which is,

In HTTP/1, if you see upgrade: x, y, z the application receives env['rack.protocol'] = ['x', 'y', 'z']. Then, the application selects one of x, y, or z and responds with {'rack.protocol' => 'y'} as the response headers. The status and other details must be correct for the SERVER_PROTOCOL advertised in the request env.

In HTTP/2, if you see :method CONNECT and :protocol x the application receives env['rack.protocol'] = ['x']. The application should respond with {'rack.protocol' => 'x'} but HTTP/2 ignores this.

In both cases, you will need to negotiate the response headers appropriately according to the specific RFCs for the protocol you are handling.

Puma itself would need to support this mapping however it could leave HTTP_UPGRADE in tact if that preserves existing behaviour, while also providing rack.protocol. It would also need to handle 101 responses with a {'rack.hijack' => proc} response headers for doing the actual streaming.

What do you think of such an approach? Can you suggest something better?

@MSP-Greg
Copy link
Member

A long time ago, I knew the differences between the draft WebSocket specs/recs. I'm interested in this, but I need a big refresh on the spec.

Does WebSocket exist with HTTP/3? Given that it is a different protocol (UDP), not sure...

@ioquatix
Copy link
Contributor Author

A long time ago, I knew the differences between the draft WebSocket specs/recs. I'm interested in this, but I need a big refresh on the spec.

There are two specs: HTTP/1 uses connection: upgrade -> 101. HTTP/2 uses :method CONNECT and :protocol websocket -> 200.

Does WebSocket exist with HTTP/3? Given that it is a different protocol (UDP), not sure...

Yes, but the stream is handled by HTTP/2+ multiplexing. The underlying transport doesn't matter to the websocket protocol itself (much).

@wtn
Copy link

wtn commented Apr 8, 2023

In my testing, Async::WebSocket::Adapters::Rails#open works in Puma 6.1.0 and later. From the commit logs, it appears the fix came in 6da32ca.

Can this issue be closed?

@ioquatix ioquatix closed this as completed Apr 8, 2023
@ioquatix
Copy link
Contributor Author

ioquatix commented Apr 8, 2023

Yes, thanks for checking!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants