Streaming bodies, PR #2740, WebSocket

[MSP-Greg]

Describe the bug

PR #2740 added support for streaming bodies. Still reviewing, but somewhat confused. Code below after the PR:

if res_body && !res_body.respond_to?(:each)
  response_hijack = res_body
else
  response_hijack = resp_info[:response_hijack]
end

cork_socket socket

# ** removed existing 'no_body code', doesn't affect this issue

if content_length
  io_buffer.append CONTENT_LENGTH_S, content_length.to_s, line_ending
  chunked = false
elsif !response_hijack and resp_info[:allow_chunked]
  io_buffer << TRANSFER_ENCODING_CHUNKED
  chunked = true
end

io_buffer << line_ending

if response_hijack
  fast_write_str socket, io_buffer.to_s
  response_hijack.call socket
  return :async
end

Note the line elsif !response_hijack and resp_info[:allow_chunked]. This means that if Content-Length is specified, we will write the header, but if resp_info[:allow_chunked] is set, there will be no transfer-eoncoding header written. Hence, the added test in test_puma_server.rb test_streaming_body is an invalid response. So, should Puma be writing the header? This could be changed by removing !response_hijack from the elsif conditional.

Also, I'm not sure if Puma should be looking for headers returned from the app that would indicate a WebSocket upgrade and changing the response headers accordingly...

@ioquatix ? Hope.this.made.sense.

[ioquatix]

Thanks for creating this issue.

I would suggest you open a corresponding PR in the rack conformance test suite https://github.com/socketry/rack-conform, it will test it against several different web servers. You could introduce a test for the failing behaviour and see how other servers handle it. It would be great to have more streaming tests, especially if there are some edge cases which need to be considered more carefully.

I am personally of the opinion that less special snowflake behaviour, the more robust, easier to understand the implementation will be, so hopefully somewhat understandably, I get a bit frustrated when I see a lot of complex logic to handle application <-> protocol mappings. My general thesis, is to try and avoid complexity, or when unavoidable, keep the complexity in well defined layers so that at least in insolation it's understandable.

With that in mind (sorry for the spiel), WebSockets are one area of the spec which introduce a ton of complexity and are hard to layer correctly. Not only that, but the semantics are protocol version specific. It's possibly one of the most frustrating areas of the HTTP spec, especially given that HTTP itself is fully bi-directional and WebSockets appear to be invented to solve a problem in the browser implementation (a bit like WebTransport) rather than a problem in the spec (chrome devs have essentially confirmed this w.r.t. full-duplex fetch).

For HTTP/1.1, WebSockets are a "connection upgrade" which requires the server to provide the raw socket to application code which implements the WebSocket protocol (or any other protocol that was upgraded, for that matter). In HTTP/2 there is a specific "connect with protocol" extension which is semantically overlaps but is also disjoint from the HTTP/1 upgrade mechanism.

For HTTP/1.1, the request can include an upgrade: x, y, z header, where x, y, z are protocols. The server is then expected to reject (4xx) or accept (101) with a specific protocol upgrade: y. After that, it should be raw IO.

In HTTP/2, content-length is never required, and is instead optional. Since all HTTP/2 bodies are effectively chunked, it is a semantic superset (since HTTP/1 chunked response body can't include content-length).

  def test_streaming_body
    server_run do |env|
      body = lambda do |stream|
        stream.write("Hello World")
        stream.close
      end

      [200, {}, body]
    end

    data = send_http_and_read "GET / HTTP/1.0\r\nConnection: close\r\n\r\n"

    assert_equal "Hello World", data.split("\r\n\r\n", 2).last
  end

This test is already in rack-conform and I'm testing Puma 5.x and 6.x: https://github.com/socketry/rack-conform/blob/de148ec410a4f3b92c3e5a4f4481ed2c52fc6dbd/test/rack/conform/streaming/body.rb#L11-L20 and it appears to be passing on all Rack 3 servers: https://github.com/socketry/rack-conform/actions/runs/3249379674/jobs/5331719317

the line elsif !response_hijack and resp_info[:allow_chunked]. This means that if Content-Length is specified, we will write the header, but if resp_info[:allow_chunked] is set, there will be no transfer-encoding header written.

I think you can decide what to prioritise here. In HTTP/1, there are several different permutations of "valid response" depending on what you prioritise. For example, trailers are only compatible with chunked encoding... so if you have a content-length, you have to decide, do I prefer content-length: x + data or transfer-encoding: chunked + chunks + trailers. https://github.com/socketry/protocol-http1/blob/e6a9235102986a7a5462aea251f2fc9cdc00d65b/lib/protocol/http1/connection.rb#L352-L381 shows the logic I'm following.

The nature of streaming is that you almost certainly don't know the content length. However, I never trust the content-length from the user application. It's an incredibly dangerous header to send an incorrect value to the client (framing offset/alignment attacks, etc).

So, should Puma be writing the header? This could be changed by removing !response_hijack from the elsif conditional.

I think this again is up to your decision. There is, in theory, nothing wrong with converting a response to use transfer-encoding: chunked - but this doesn't work for upgrade requests.

Falcon (async-http) handles this explicitly by tracking the protocol of the response. I'm not a fan of this design, but it was the best I could come up with: https://github.com/socketry/async-http/blob/65e941545055350c60b62e43f34a81154dc4ee00/lib/async/http/protocol/http1/server.rb#L58-L123.

The specific one you care about is this: https://github.com/socketry/async-http/blob/65e941545055350c60b62e43f34a81154dc4ee00/lib/async/http/protocol/http1/server.rb#L79-L85 where essentially we check whether it's an "upgrade response" (has a protocol) and if so, handles that case specifically (streaming the response directly without any chunked encoding, etc.

Back to my original point, this is one thing I find frustrating, because we have to be very specific with how we handle the response, there is a general logic, but there are a lot of specific snowflakes we have to handle.

Unfortunately, the same is probably true for Puma - in that you might need to explicitly detect and handle the upgrade code path. This means you should be detecting that the response is an upgrade and writing the correct response back out - i.e. it shouldn't have content length or chunked encoding.

I wanted to expose this as a more general model, e.g. rack/rack#1946 but need at least two servers to implement it (falcon does) before I can make it an official part of the Rack spec. If you want, we can try to implement this (and add tests to the conformance test suite to validate it).

[ioquatix]

I took a look at the rack-conform streaming body test on Puma v6:

> curl -v http://localhost:9292/streaming/body
*   Trying 127.0.0.1:9292...
* Connected to localhost (127.0.0.1) port 9292 (#0)
> GET /streaming/body HTTP/1.1
> Host: localhost:9292
> User-Agent: curl/7.85.0
> Accept: */*
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
* no chunk, no close, no size. Assume close to signal end
< 
* Closing connection 0
Hello World⏎   

I can't fault this response, what are you thinking is invalid about it?

[dentarg]

I wanted to expose this as a more general model, e.g. rack/rack#1946 but need at least two servers to implement it (falcon does) before I can make it an official part of the Rack spec. If you want, we can try to implement this (and add tests to the conformance test suite to validate it).

We have #2914 for that, right?

[ioquatix]

Yes, but that's a more specific issue/bug in relation to formulating valid upgrade responses directly. I think the proposal I gave more in relation to how we handle this generally (and what we want in the rack spec). I would like to either:

• Discuss the proposal and come up with a counter proposal, OR
• Accept the proposal and try it out in puma.

Jeremy Evans has indicated that he will accept the feature into the rack spec if at least two servers implement it.

The alternative is to be more specific to HTTP/1 and HTTP/2 but I feel that this is a bad approach because it exposes user code to more complexity.

I think I've already outlined the major differences, but in summary:

• HTTP/1 requires method GET, connection: upgrade and upgrade: x, y, z in the request, and upgrade: y in the response with status 101 to accept.
• HTTP/2 requires method CONNECT, :protocol: y in the request, and a response with status 200 to accept.

My proposal is to implement the intersection. Recall that Rack roughly implements the CGI specification with rack specific extensions. I also have a strong feeling that we should map 200 okay to 101 when dealing with HTTP/1 upgrades, so that applications also don't need to care about that detail. (1xx responses are defined as informational non-final response codes, so 101 for what amounts to a final response code is a really confusing idea and I believe it's a really rough part of the HTTP/1 spec).

[MSP-Greg]

I can't fault this response, what are you thinking is invalid about it?

I thought either content-length needed to be set or Transfer-Encoding: chunked needed to exist. I guess not.

Re WebSockets, got the spec on my iPad, brushing up on it.

Re rack/rack#1946, is it okay to have both HTTP_UPGRADE' and rack.protocol sent to the app?

[ioquatix]

I thought either content-length needed to be set or Transfer-Encoding: chunked needed to exist. I guess not.

Right - IIRC, HTTP/1 assumes connection: close if neither content-length nor transfer-encoding: chunked is specified. Closing the connection indicates the end of the data... or a failure in the network - it's impossible to tell and thus this is a potential problem for the client).

Re WebSockets, got the spec on my iPad, brushing up on it.

Nice - let me know if you have any questions.

Re rack/rack#1946, is it okay to have both HTTP_UPGRADE' and rack.protocol sent to the app?

I think my goal is to avoid applications having to deal with the nuances of the underlying protocol... Since HTTP/1 and HTTP/2 are wildly different in this regard, I'd like to see if we can't have a standard "this request is trying to negotiate this protocol" and "this response is using this protocol" without dealing in the specific headers, status codes and other details. Therefore, I'd prefer if we don't forward HTTP_UPGRADE and instead opt for a better way which is less HTTP-version specific. (HTTP/2 does not use the upgrade header and it's explicitly disallowed in the H2 spec).

[MSP-Greg]

we don't forward HTTP_UPGRADE

But, if current app code is expecting HTTP_UPGRADE and HTTP_CONNECTION headers, removing/translating them may break things?

I think my goal is to avoid applications having to deal with the nuances of the underlying protocol

I understand that, and I appreciate your work re HTTP/2. Ruby's (and probably other languages) 'web' ecosystem developed with clear lines of responsibility for the pieces. HTTP/2 and HTTP/3 blow that all apart.

Just noticed Wikipedia's page for HTTP/3

"HTTP/3 is supported by 75% of web browsers (and 83% of 'tracked desktop' web browsers) and 26% of the top 10 million websites."

[ioquatix]

But, if current app code is expecting HTTP_UPGRADE and HTTP_CONNECTION headers, removing/translating them may break things?

Unfortunately there is no really good solution.

The connection header is part of the protocol and should never be an application level concern IMHO. The same could be said for the upgrade header.

That's why I'd like to have the discussion with someone who has time to find the right solution (e.g. yourself). I'm not against some kind of middle ground for compatibility sake, as long as we know what direction we are ultimately going in.

I understand that, and I appreciate your work re HTTP/2. Ruby's (and probably other languages) 'web' ecosystem developed with clear lines of responsibility for the pieces. HTTP/2 and HTTP/3 blow that all apart.

HTTP/2 and HTTP/3 semantic model are much more similar than HTTP/1 and HTTP/2. So, HTTP/3 is a non-issue (assuming we solve for the HTTP/1 and HTTP/2 cases).

The real issue here is that most ALBs speak CGI or HTTP/1 or some mix. So applications themselves still have to deal with some of the less well thought out semantics of HTTP/1 even if the client itself is speaking HTTP/2 or HTTP/3.

[dentarg]

What needs to be done to close this issue? It isn't immediately clear to me, but I think #3007 (comment) tries to answer it. Can we adjust the title of this issue so it becomes more clear?

We do already have an issue about WebSocket support: #1316

[ioquatix]

I've validated the implementation in protocol-rack gem, which will accept both HTTP_UPGRADE and rack.protocol. That means, in theory, async-websocket can work on Puma.

However, in practice, the puma tests here are failing: socketry/rack-conform#12.

If we can get those tests passing that would be a good step forward.

[MSP-Greg]

@ioquatix

Thanks for your work on this.

If we can get those tests passing that would be a good step forward.

I'll have a look early this week...

[MSP-Greg]

@ioquatix

Running against rack-conform master and Puma PR #3004, Actions run.

I assume I should be working with the websockets branch?

[ioquatix]

I tried running it locally and it was passing, so I need to check a little bit more.

[ioquatix]

Okay, I wonder if when running on my M1 Mac, it's not using native extensions, and so it passes, but when using the native extensions on x86/Linux it fails.

Anyway, the error on Linux:

2022-11-14 18:12:39 +1300 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?>

It passes on my M1 Mac.

To run the tests:

export BUNDLE_GEMFILE=gems/puma-head-rack-v3.rb
export RACK_CONFORM_SERVER="puma --bind tcp://localhost:9292"
export RACK_CONFORM_ENDPOINT="http://localhost:9292"
bundle exec sus

[ioquatix]

I assume I should be working with the websockets branch?

Yes, it should be.

[MSP-Greg]

@ioquatix - simple fix, but haven't added it yet. There's a value in Puma:

resp_info[:no_body] ||= status < 200 || STATUS_WITH_NO_ENTITY_BODY[status]

Note the status < 200. WS starts with a 101, but with resp_info[:no_body] being true, it assumed there was no response body, and without a body, there's nothing to call.

See https://github.com/MSP-Greg/rack-conform/tree/00-puma6-ws a few extra commits to run in my fork
uses https://github.com/MSP-Greg/puma/tree/00-size-to-first-byte-ws, which is based on PR #3004, with one additional commit for the WS issue...

[ioquatix]

It looks okay to me. I left a minor feedback.

[ioquatix]

@MSP-Greg is there a plan to merge this soon?

[MSP-Greg]

@ioquatix Sorry, busy tearing apart some old code. Yes, I think it should be merged soon.

[ioquatix]

How we going on this? I'm working on WebSocket support directly within Rails so would be good to test it on Puma too.

[ioquatix]

Awesome, great work team!

[ioquatix]

I've tested rack-conform on puma head and it's now passing: https://github.com/socketry/rack-conform/actions/runs/4021779966/jobs/6937352800

However, as expected, v6 is not: https://github.com/socketry/rack-conform/actions/runs/4021779966/jobs/6937352748

Do you think we can do a v6.x minor release? Thanks!

[nateberkopec]

Yup, it's about time for one.

[ioquatix]

Can you let me know when it's done so I can rerun the tests on my end! Just mention me in the PR/commit/whatever :)

[adenta]

what ever happened here?