Add pitchfork on Rack 3.

[ioquatix]

cc @Earlopain @byroot

Types of Changes

• New feature.

Contribution

• I added tests for my changes.
• I tested my changes locally.
• I agree to the Developer's Certificate of Origin 1.1.

[Earlopain]

Is websocket support optional? I think this is the corresponding spec PR rack/rack#1954 formalizing it.

I see your PR for puma at puma/puma#3399, but then it looks like it supported websockets a while ago already puma/puma#3399

[ioquatix]

rack.protocol is not needed for websockets, only correct handling of connection upgrade. I just fixed this in protocol-rack and it fixed puma's tests here:

It should be the same for Pitchfork... let me check the errors.

[ioquatix]

Here is the response we are generating on the server:

{:response=>
  [101,
   {"sec-websocket-extensions"=>
     ["permessage-deflate;client_max_window_bits=15"],
    "sec-websocket-accept"=>["2RWck3vRgnko6Ww+leXxh3c/nhU="],
    "upgrade"=>"websocket",
    "connection"=>"upgrade"},
   #<Method: Async::HTTP::Body::Hijack#call(stream) /Users/samuel/.gem/ruby/3.3.2/gems/async-http-0.67.1/lib/async/http/body/hijack.rb:38>]}

And here is how it ends up on the client:

{:version=>"HTTP/1.1",
 :status=>101,
 :headers=>
  #<Protocol::HTTP::Headers [["Date", "Fri, 14 Jun 2024 16:56:46 GMT"], ["Connection", "close"], ["sec-websocket-extensions", "permessage-deflate;client_max_window_bits=15"], ["sec-websocket-accept", "2RWck3vRgnko6Ww+leXxh3c/nhU="], ["upgrade", "websocket"]]>}
{:body=>nil}

From what I can see, for some reason, the connection header is not set correctly.

[Earlopain]

Unicorn/Pitchfork hardcode the connection header https://github.com/Shopify/pitchfork/blob/79ba8e9f0468829af18b46027c123f9d5bcb3dd8/lib/pitchfork/http_response.rb#L53-L58

[ioquatix]

Nice find. I guess that's the problem then.

[ioquatix]

There are two options:

1. Correctly handle response with connection: upgrade.
2. If pitchfork wants to have more control over the upgrade process, it should use rack.protocol.

[byroot]

There is no point supporting websockets in Pitchfork or Unicorn. They entirely go against what it's built for. Pitchfork will remain a pure HTTP/1.1 server.

[ioquatix]

@byroot We are talking about connection: upgrade, not websockets specifically. Websockets is just a reasonable test for that functionality.

In any case, if you don't want to support it, you should delete the HTTP_UPGRADE in the request header before invoking the app, otherwise there is no way for apps to know that you don't support it.