Support Rack 3.x

[lloeki]

From #389 here's a first draft that makes Thin specs pass.

There's a warning that should probably be addressed:

warning: Rack::File is deprecated and will be removed in Rack 3.1

rg Rack::File
lib/rack/adapter/rails.rb
26:        @file_app  = Rack::File.new(::File.join(RAILS_ROOT, "public"))

lib/rack/adapter/loader.rb
67:        return Rack::File.new(options[:chdir])

example/adapter.rb
23:    run Rack::File.new('.')
30:#                          '/files' => Rack::File.new('.'))

spec/rack/loader_spec.rb
35:    expect(Rack::File).to receive(:new)

[lloeki]

Here's a quick run of https://github.com/socketry/rack-conform as was suggested by @ioquatix:

6 passed 2 errored out of 8 total (11 assertions)
🏁 Finished in 5.9ms; 1854.349 assertions per second.
🐢 Slow tests:
	454.7ms: server "thin start -a localhost -p 9292" http://localhost:9292

🔥 Errored assertions:
file test/rack/conform/streaming/body.rb it can stream a response test/rack/conform/streaming/body.rb:12
	⚠ EOFError: Could not read line!
file test/rack/conform/websocket.rb it can establish a websocket connection test/rack/conform/websocket.rb:15
	⚠ EOFError: Could not read line!

The internal error is:

Unexpected error while processing request: undefined method `each' for #<Proc:0x00000001045ba680 github.com/socketry/rack-conform/lib/rack/conform/application.rb:78>
	github.com/macournoyer/thin/lib/thin/response.rb:96:in `each'
	github.com/macournoyer/thin/lib/thin/connection.rb:118:in `post_process'
	github.com/macournoyer/thin/lib/thin/connection.rb:53:in `process'
	github.com/macournoyer/thin/lib/thin/connection.rb:39:in `receive_data'
	.gem/ruby/3.2.0/gems/eventmachine-1.2.7/lib/eventmachine.rb:195:in `run_machine'
	.gem/ruby/3.2.0/gems/eventmachine-1.2.7/lib/eventmachine.rb:195:in `run'
	github.com/macournoyer/thin/lib/thin/backends/base.rb:75:in `start'
	github.com/macournoyer/thin/lib/thin/server.rb:162:in `start'
	github.com/macournoyer/thin/lib/thin/controllers/controller.rb:87:in `start'
	github.com/macournoyer/thin/lib/thin/runner.rb:203:in `run_command'
	github.com/macournoyer/thin/lib/thin/runner.rb:159:in `run!'
	github.com/macournoyer/thin/bin/thin:6:in `<top (required)>'
	.gem/ruby/3.2.0/bin/thin:25:in `load'
	.gem/ruby/3.2.0/bin/thin:25:in `<main>'

Pointing to Response#each doing:

    def each
      yield head

      unless @skip_body
        if @body.is_a?(String)
          yield @body
        else
          @body.each { |chunk| yield chunk }    # <=== here
        end
      end
    end

[lloeki]

This is fixed:

file test/rack/conform/streaming/body.rb it can stream a response test/rack/conform/streaming/body.rb:12
	⚠ EOFError: Could not read line!

The last error is more about implementing WebSocket support in Thin than any Rack 3 compliance, which I feel is an entirely separate task:

file test/rack/conform/websocket.rb it can establish a websocket connection test/rack/conform/websocket.rb:15
	⚠ EOFError: Could not read line!

I tried some hacky things, but eventually with the test design it's about getting access to the socket behind EventMachine and passing it down for direct handling.

Tangent: it seems for that part the conformance test implementation is heavily biased towards the Falcon implementation of WebSockets (like, it excludes everyone but Falcon to test against Rack 3)

[ioquatix]

Tangent: it seems for that part the conformance test implementation is heavily biased towards the Falcon implementation of WebSockets (like, it excludes everyone but Falcon to test against Rack 3)

I don't recall any such limitation, can you clarify what limitation you have found?

[lloeki]

Huh I can't find it now. Maybe I got confused.

[ioquatix]

That's okay. It's been one of the more tricky parts of the spec to get consistency on, and all Rack 3 conforming servers should be capable of supporting live WebSockets per request/response using streaming response body. Feel free to make a PR to rack-conform to start testing thin (you can even specify a branch).

[lloeki]

Heh I already have a branch locally, I just need to push and create the PR there :)

I'm a bit surprised about Rack 3 mandating WS support, but whatever. I'll try to figure out something once I get a moment to spare for that.

[ioquatix]

I'm a bit surprised about Rack 3 mandating WS support

There is nothing specific in Rack 3 about WebSockets, it's just a good test of streaming responses. There is very little difference between that and partial rack.hijack (semantically, they are almost the same thing).

[arikarim]

Thanks for your hard work.

any update on this?

[ioquatix]

I think the requirements to merge this would be:

1. All tests passing.
2. rack-conform test suite passing.

[ioquatix]

If supporting old releases of Ruby is too much work, we could drop them. I'd be okay with dropping all EOL Rubies for the next release.

[lloeki]

rack-conform test suite passing.

which means websocket support

any update on this?

I've been:

• exploring eventmachine a bit to either directly get to the socket or hack an event proxy to it
• punching at this (unidirectional) each which leads to a bit of an odd code in the (bidirectional) websocket case.

Not much time/energy to do this lately. Not giving up though.

EOL Rubies

Could make sense, seems like a sensible option but first things first, let's make websockets pass.

[ioquatix]

which means websocket support

You only need to support streaming responses, which is not the same as WebSockets. It's the same as partial hijack. Even puma can support it with threads.

[ioquatix]

IIRC, you should use SERVER_PROTOCOL for this. See https://github.com/rack/rack/blob/main/SPEC.rdoc for more details.

[lloeki]

Thin internals relied on HTTP_VERSION. Removing it would have broken things so I added this so as not to mess with things too much while I got clarification.

IIUC you're saying:

• this logic here should rely on SERVER_PROTOCOL instead of HTTP_VERSION
• thin.request_http_version can then disappear entirely

Correct?

[lloeki]

which means websocket support

You only need to support streaming responses, which is not the same as WebSockets. It's the same as partial hijack. Even puma can support it with threads.

I'm not that good with websockets, but AIUI this rack-conform test (the last one failing) does bidirectional access to the socket:

• https://github.com/socketry/rack-conform/blob/e2382a165775275c1cdbb6c8a1cb7ed15532d2cc/lib/rack/conform/application.rb#L89-L96
• https://github.com/socketry/rack-conform/blob/e2382a165775275c1cdbb6c8a1cb7ed15532d2cc/test/rack/conform/websocket.rb#L19-L21

To pass this test, the underlying EventMachine connection must be somehow made available for Async::WebSocket::Adapters::Rack.open to peruse, upgrade the transport to WS, then pass connection to the block.

[ioquatix]

To pass this test, the underlying EventMachine connection must be somehow made available for Async::WebSocket::Adapters::Rack.open to peruse, upgrade the transport to WS, then pass connection to the block.

Yes, that's correct. It's almost identical to partial hijack. Does thin support partial hijack?

[ioquatix]

See:

• Support for Rack hijack specification #157
• Compatibility with ActionCable #298

It looks like thin never supported partial hijack.

It became a requirement for Rack 3.

I understand that EM does not support bare non-blocking IO.

I would advise that EM can be updated to use the fiber scheduler to solve this problem, and thin can adopt that model to provide proper hijack support (event driven IO).

I believe this already exists, but I don't have much experience with it: https://git.singpolyma.net/em_fiberscheduler

[lloeki]

Interesting. Thanks for these pointers.

Overall the plan makes sense to me and although I did not anticipate things to such detail it matches what I broadly expected.

Coincidentally I've recently toyed with https://github.com/bruno-/fiber_scheduler a little so EM:FiberScheduler somewhat makes sense to me.

I have little practical experience with hijack and none about partial hijack but I'll dig into it.

So, setting expectations for people following this: it means I now have a bunch of solid known unknowns to explore, which I will tackle eventually but will take me a bit of time.

[womblep]

It looks like thin never supported partial hijack.

It became a requirement for Rack 3.

where is partial hijack documented as a requirement for Rack 3?
I can only see it being optional and only as backwards compatibility
From the Rack Spec:
Full hijacking only works with HTTP/1. Partial hijacking is functionally equivalent to streaming bodies, and is still optionally supported for backwards compatibility with older Rack versions.

According to the Rack upgrade doc, Websockets can be supported using streaming bodies as described here https://github.com/rack/rack/blob/main/UPGRADE-GUIDE.md#response-bodies-can-be-used-for-bi-directional-streaming

@lloeki I would be inclined to push the change without the websocket test passing (which selfishly suits my use-case :-) ) and look at Websockets as a separate item

[ioquatix]

Partial hijack and streaming response bodies are semantically identical.

See https://github.com/rack/rack/blob/main/SPEC.rdoc#streaming-body- for a clarification.

[lloeki]

Dropping some notes.

Rack reaches down to test_websocket_echo, which calls Async::WebSocket::Adapters::Rack.open:

https://github.com/socketry/rack-conform/blob/e2382a165775275c1cdbb6c8a1cb7ed15532d2cc/lib/rack/conform/application.rb#L90

Async::WebSocket::Adapters::Rack.open receives the Rack env and wraps that in a ::Protocol::Rack::Request

https://github.com/socketry/async-websocket/blob/5c9349081b0db850bd27587233e2bd20bef616fb/lib/async/websocket/adapters/rack.rb#L23

Then performs HTTP.open:

https://github.com/socketry/async-websocket/blob/5c9349081b0db850bd27587233e2bd20bef616fb/lib/async/websocket/adapters/rack.rb#L25

Which validates a bunch of things and ultimately calls Response.for (with the block that will handle WS lifecycle):

https://github.com/socketry/async-websocket/blob/5c9349081b0db850bd27587233e2bd20bef616fb/lib/async/websocket/adapters/http.rb#L37

Response.for calls in to prepare the upgrade over HTTP/1:

https://github.com/socketry/async-websocket/blob/5c9349081b0db850bd27587233e2bd20bef616fb/lib/async/websocket/response.rb#L17

UpgradeResponse sets the 101 switching protocols status + headers + body:

https://github.com/socketry/async-websocket/blob/5c9349081b0db850bd27587233e2bd20bef616fb/lib/async/websocket/upgrade_response.rb#L29

The UpgradeResponse instance bubbles up as a return value all the way through test_websocket_echo and thus is semantically the Rack response.

The response body actually captures the WS lifecycle block:

https://github.com/socketry/async-websocket/blob/5c9349081b0db850bd27587233e2bd20bef616fb/lib/async/websocket/upgrade_response.rb#L27

The body is a callable that forwards its stream argument to the WS lifecycle block:

https://github.com/socketry/async-http/blob/260d8272384d88945fee090c5e9df4bf8b2be0c7/lib/async/http/body/hijack.rb#L38-L40

And a readable that lazily sets up a ::Protocol::HTTP::Body::Stream.new to forward to the WS lifecycle block:

https://github.com/socketry/async-http/blob/260d8272384d88945fee090c5e9df4bf8b2be0c7/lib/async/http/body/hijack.rb#L54-L67

The body's @input is set to be the request body:

https://github.com/socketry/async-http/blob/260d8272384d88945fee090c5e9df4bf8b2be0c7/lib/async/http/body/hijack.rb#L21

Thin sees the UpgradeResponse which is a Protocol::HTTP::Response:

https://github.com/socketry/protocol-http/blob/2d4e6eff803d80116f1044785ffc5af1b2711727/lib/protocol/http/response.rb#L111

Before Thin sees the response, it gets transformed via Protocol::Rack::Adapter.make_response:

https://github.com/socketry/async-websocket/blob/main/lib/async/websocket/adapters/rack.rb#L26

Which makes it the usual Rack response array:

https://github.com/socketry/protocol-rack/blob/5862e7f9fab3f8ebb5fe9622bfec6eb120cb0800/lib/protocol/rack/adapter/rack3.rb#L104

But not before transforming the body callable into a proc:

https://github.com/socketry/protocol-rack/blob/5862e7f9fab3f8ebb5fe9622bfec6eb120cb0800/lib/protocol/rack/adapter/rack3.rb#L101

Because it declares itself as being a streaming response:

https://github.com/socketry/async-http/blob/260d8272384d88945fee090c5e9df4bf8b2be0c7/lib/async/http/body/hijack.rb#L34-L36

So Thin sees:

[101, { some: headers }, method(Async::HTTP::Body::Hijack#call)]

According to Rack a body responding to #call but not #each means this is a "Streaming Body", which is to be sent :call with a stream argument, which is what happens now:

https://github.com/macournoyer/thin/pull/399/files#diff-7e2b83e2b5768e153d12ab91052330047ea8dd3c61ff3ef3538b2553ab420b0bR144

But this goes through:

https://github.com/lloeki/thin/blob/96610068275be3fd3901133fdec800512d42fe94/lib/thin/connection.rb#L50

preprocess involves setting a default AsyncResponse and listening for a throw(:async) from the app:

https://github.com/lloeki/thin/blob/96610068275be3fd3901133fdec800512d42fe94/lib/thin/connection.rb#L84-L92

AsyncResponse is a fake response with -1 as status code:

https://github.com/lloeki/thin/blob/96610068275be3fd3901133fdec800512d42fe94/lib/thin/connection.rb#L11

Which comes into play in postprocess to bail out with nothing:

https://github.com/lloeki/thin/blob/96610068275be3fd3901133fdec800512d42fe94/lib/thin/connection.rb#L103-L104

But currently this code path is unused (throw :async is expected to come from the app) and we fall through to calling Thin::Response#each in a way that largely expects a chunked, one-way streaming response:

https://github.com/lloeki/thin/blob/96610068275be3fd3901133fdec800512d42fe94/lib/thin/connection.rb#L118-L121

[lloeki]

So AIUI we'd leverage partial hijack to have Thin itself return UpgradeResponse's status 101 + headers via Rack, but ignoring the body (in terms of having Thin processing it itself).

Instead partial hijack support would have the "body" (which is really a wrapper for WS stream handling) be called with a stream (really the connection stream be handed over) in a separate specific case before we reach this:

https://github.com/lloeki/thin/blob/b428f6cfeaac1978c0de487d45c3357a8d0ad773/lib/thin/connection.rb#L117

That, or Thin::Response#each becomes a call or something handling both cases, and Thin::Response handles the entirety of the Rack protocol negotiation about what happens depending on what the body responds to, e.g the block's trace + send_data would be wrapped in a stream object and passed to call, thus being fitted into a more general case of general streaming support. IOW in terms of interface between Thin::Connection and Thin::Response instead of attempting to pretend WS are one-way stream like (which each suggests) we have one-way streaming fit into two-way streaming (but with the read side closed).

In both cases Thin handles the status + headers for switching protocols then hands things over one way or another.

[lloeki]

which means websocket support

You only need to support streaming responses, which is not the same as WebSockets. It's the same as partial hijack. Even puma can support it with threads.

Oh I realise that my comment was ambiguous: indeed what I meant by "WS support" is "do the thing in Thin that enables use of WS for the test to pass" which indeed means partial hijack if the non-Thin WS thing delegates 101 to Rack (which AIUI is the case for this rack-conform test) or theoretically full hijack if the non-Thin WS thing handles that itself by writing it directly to the connection (which is not the case for this test).

Sorry for the confusion.

[womblep]

That, or Thin::Response#each becomes a call or something handling both cases, and Thin::Response handles the entirety of the Rack protocol negotiation about what happens depending on what the body responds to, e.g the block's trace + send_data would be wrapped in a stream object and passed to call, thus being fitted into a more general case of general streaming support.

I think that is a better, more generic approach.

And if you set a streaming flag in Thin::Response for the case where you are dealing with a streaming body, then you can have a check in Thin:Connection->receive_data where you check the streaming flag and if set buffer the data in the stream, bypassing the pre/post_process logic. That handles the 2 way streaming support.

You would probably have to skip the terminate_request in that case too, and call that somehow once you get a close from the stream

thin/lib/thin/connection.rb

Line 134 in 3f37538

terminate_request unless result && result.first == AsyncResponse.first