WebSocket support?

[dentarg]

Description

I've rebased @MSP-Greg's branch https://github.com/MSP-Greg/puma/tree/00-size-to-first-byte-ws

With these changes, Puma passes the Rack::Conform "Basic WebSocket connection test", see https://github.com/dentarg/rack-conform/actions/runs/3928289028/jobs/6715741130

Needs some tests

Close #3007

Your checklist for this pull request

• I have reviewed the guidelines for contributing to this repository.
• I have added (or updated) appropriate tests if this PR fixes a bug or adds a feature.
• My pull request is 100 lines added/removed or less so that it can be easily reviewed.
• If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
• If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
• I have updated the documentation accordingly.
• All new and existing tests passed, including Rubocop.

[MSP-Greg]

@dentarg

Thanks. I intended to spend yesterday working on Puma, then a Windows thing came up. From the Actions Windows Ruby 3.1 log:

                         Puma::MiniSSL                   OpenSSL
OPENSSL_LIBRARY_VERSION: OpenSSL 3.0.7 1 Nov 2022        OpenSSL 1.1.1s  1 Nov 2022
        OPENSSL_VERSION: OpenSSL 3.0.7 1 Nov 2022        OpenSSL 1.1.1s  1 Nov 2022

It passes CI, but having Ruby and Puma compile with different OpenSSL versions is not something we want happening. This has nothing to do with Puma, it's all in the code setting up GitHub Actions for Ruby.

Fine print: The MSYS2 system, which provides the build tools and packages used to build most Windows Rubies, updated their OpenSSL package from 1.1.1s to 3.0.7.

[nateberkopec]

Amazing work @dentarg! Thank you for doing this.

[MSP-Greg]

@dentarg Thanks for working on this. Just added an Actions workflow that uses rack_conform to test Puma. Right now, it can 'silently' fail, not sure if that should be removed?

@ioquatix This PR fixes Puma websocket support, and adds a workflow that uses the websockets branch of rack_conform to test Puma. Anyway to get a list of the tests? I assume at some point we'll switch to the main branch. Note that the workflow 'hacks' the gemfile as below, $GITHUB_REPOSITORY let's it run in a fork of Puma.

      - name: Update gems/puma-head-rack-v3.rb
        run: |
          SRC="gem \"puma\", git: \"https://github.com/puma/puma.git\""
          DST="gem 'puma', git: 'https://github.com/$GITHUB_REPOSITORY.git', ref: '$GITHUB_SHA'"
          sed -i "s#$SRC#$DST#" gems/puma-head-rack-v3.rb
          cat gems/puma-head-rack-v3.rb

[ioquatix]

I would also be okay with adding local test gem files to rack-conform if that makes it easier - can you not use the form gem "puma", path: "../" or something like that?

It would be neat if we could figure out how to use https://github.com/ioquatix/bake-test-external to do this orchestration more easily because It would be awesome to encourage other servers to adopt rack-conform test suite.

[MSP-Greg]

@ioquatix Please allow the Window's guy some slack with sed. My original sed strings were terrible, updated to:

SRC="gem ['\"]puma['\"].*"
DST="gem 'puma', git: 'https://github.com/$GITHUB_REPOSITORY.git', ref: '$GITHUB_SHA'"
sed -i "s#$SRC#$DST#" gems/puma-head-rack-v3.rb

I think it will be a bit more stable. I'm ok with this, as we're running the CI as if it's from the rack-conform repo, and changing the 'Gemfile' before bundle install. Need to use GITHUB_REPOSITORY, as it allows running from a fork of Puma.

[ioquatix]

This looks good to me!

[MSP-Greg]

Thanks. Please let us know when we can change to the main branch.

Don't know about everyone else, but after seeing all the strange-ass things various (external) response classes do, I think it's a good idea for Puma to test with repos like rake-conform and turbo-rails...

[ioquatix]

Okay, I will work on merging the websockets PR on rack-conform, if we can merge this one soon it will help.

[MSP-Greg]

One reason I paused on this code is the status code handling. I believe any response with a status code < 200 is considered a 'no body/content' response. IANA's Hypertext Transfer Protocol (HTTP) Status Code Registry and MDN only show four codes in that range.

But, here and there it's implied that servers/apps can/may define their own. Thoughts? This code does not handle undefined 100 'range' codes as 'no body' codes...

[MSP-Greg]

Okay, I will work on merging the websockets PR on rack-conform, if we can merge this one soon it will help.

As long as you're back in that repo, might it log the tests? It's not like there's hundreds/thousands of them...

BTW, no hurry, and thanks for getting it up and running. Should be in the rack account, but maybe that's planned for later. And, any other servers can feel free to use the workflow in their CI...

[ioquatix]

@jeremyevans what do you think about moving the rack-conform test suite to the rack org?

[ioquatix]

But, here and there it's implied that servers/apps can/may define their own. Thoughts? This code does not handle undefined 100 'range' codes as 'no body' codes...

I've argued this within the rack spec, since Rack::Response also assumes any response of status code 100 doesn't have an entity body and thus was stripping out the upgrade/streaming response body.

rack/rack#1987

The current implementation assumes that a streaming body is a super-set of entity bodies, and thus you shouldn't prune the response body for 1xx responses with a streaming response body, and was implemented in rack/rack#1993

[jeremyevans]

@jeremyevans what do you think about moving the rack-conform test suite to the rack org?

I think we should be more conservative when adding new repositories to the rack org. In hindsight, I don't think rack-test or rack-attack should have been moved to it.

After looking at the rack-conform repository and dependencies, I'm not in favor of moving rack-conform to the rack org. In general, I don't think we should move external repositories into the rack org unless we have multiple rack committers willing to maintain them.

[MSP-Greg]

unless we have multiple rack committers willing to maintain them.

Makes sense. But, as I've seen with Puma, web/app servers may be affected by small behavior changes in Rack, and they may not be testing for them. Some kind of conformance code might be helpful. Haven't really thought about it much...

[nateberkopec]

Marking as "waiting for changes" as it maybe needs a test or two in the Puma test suite.

@dentarg do you feel up to that or want me and Greg to do it? Don't want you to sign up for more than you want to! 😄

[ioquatix]

Okay, so my read is, the bar for moving rack-conform is at least one other person willing to help maintain it, whom is willing to also join the rack core team for maintenance purposes.

As it stands, it's fine where it is, but long term, eventually I have to assume I'm no longer going to be around (hopefully not for a long time). But when that happens, it makes sense that there is a way to continue maintaining the code if servers are using it as part of validating their conformance to the rack spec.

[MSP-Greg]

Marking as "waiting for changes" as it maybe needs a test or two in the Puma test suite.

JFYI, the additional workflow (using rack-conform) is creating a websocket connection, and also testing that the server (Puma) is correctly returning a 101 ('Switching Protocols') response and passing the IO to the app.

I assume that some apps may hijack the connection and handle the 101 response themselves...

[dentarg]

@nateberkopec if you or @MSP-Greg can add the tests we need, that's fine with me! Currently I don't know when I have a chunk of time free for that

[ioquatix]

I have merged the websockets branch and it is now failing for Puma as expected.

[MSP-Greg]

@dentarg @nateberkopec

Re tests, see the last commit 'Create test_puma_server_partial_hijack.rb, pull 1 test from test_puma_server.rb'. It pulls one test from test_puma_server.rb.

The two new tests fail when the commit is cherry picked to master.

[MSP-Greg]

I just added three commits, one which reverts the first commit, which is the 101 fixes. The first original commit changed the definition of resp_info[:no_body] such that new or proprietary 1xx status codes would not be included.

But, RFC 9110 15.2 clearly states "A 1xx response is terminated by the end of the header section; it cannot contain content or trailers."

The last new commit "request.rb - properly handle 101 'Switching Protocols' responses" correctly handles the above cases.

The new tests seem to be failing on TruffleRuby, possibly due to using syswrite & sysread. Not.sure. I hope to look at it sometime today.

[ioquatix]

I'd like to introduce rack.protocol which when combined with a streaming body indicates an upgrade response.

Essentially:

if protocol = headers['rack.protocol']
  headers['connection'] = 'upgrade'
  headers['upgrade'] = protocol
  send_headers(headers)
  body.call(io)
else
  # other response handling

This also helps us in HTTP/2 which has a different "upgrade" mechanism (which doesn't use 101 status code).

[MSP-Greg]

Added one more commit, fixes the TruffleRuby issue. Adding an io.wait_readable statement fixed the issue.

All passed in the test workflow. I consider this finished, any thoughts?

[ioquatix]

Okay, let me give it a review. w.r.t. my previous comment, detecting 101 status is good enough for now.

[ioquatix]

LGTM.

[MSP-Greg]

I'd like to introduce rack.protocol

Discuss in Rack? JFYI, I'm kind of meh about it, as, assuming most WebSocket is wss (ssl/tls), the app will be generating other headers anyway. So why would the app/web server need to add some more?

HTTP/2 which has a different "upgrade" mechanism

I have yet to really take a 'deep dive' into HTTP/2 (or HTTP/3), so I'm speaking mostly about HTTP/1...

[ioquatix]

Yep, we have a discussion for it: rack/rack#1946

Right now, you are expecting the response to include the connection: upgrade and upgrade: websocket headers, as well as status code 101. But those headers are invalid for HTTP/2 so applications need to version sniff to send the correct response, which isn't obvious if you don't know that it's required. It's another area where HTTP/2 compatibility can be tricky.

[ioquatix]

I've just merged and released support for webrick (v1.8.0) bi-directional streaming, added support in rackup (v2.1.0), and tested this using the conformance test suite (fully passing now for webrick on Rack 3). Looking forward to merging and releasing this PR too!! By the way, I don't care much about webrick, but it's our lowest common denominator when it comes to rackup, so it makes sense it at least works correctly (perhaps performance is less of an issue/concern).

I've added support for both rack.protocol and upgrade response headers, you can check how it works here:

https://github.com/rack/rackup/blob/eaea24a3d64a1b117df943a9d06779e659bb61af/lib/rackup/handler/webrick.rb#L137-L144

I don't actually have a strong opinion about this, but it's useful not to have to special case HTTP v1 and v2 too much, given that the upgrade header should not be specified on an HTTP/2 response.

[nateberkopec]

A few minor clarifications and then I'm ✅

[ioquatix]

Looking good!

[ioquatix]

Nice!