socketry / falcon

A high-performance web server for Ruby, supporting HTTP/1, HTTP/2 and TLS.
https://socketry.github.io/falcon/
MIT License
2.54k stars 79 forks source link

Failed to upgrade to WebSocket #202

Closed rubys closed 6 months ago

rubys commented 1 year ago

I'm hoping I missed an obvious step, and am posting here before I dive in deeper.

Full message here:

Failed to upgrade to WebSocket (REQUEST_METHOD: GET, HTTP_CONNECTION: keep-alive, Upgrade, HTTP_UPGRADE: )
Finished "/cable"[non-WebSocket] for 127.0.0.1 at 2023-03-22 08:37:13 -0600

Context: I'm exploring what it would take (if anything) to support Falcon servers running on fly.io. I started with Demo 3 which you can run on your own machine if you have Rails 7, Docker, Redis, and Postgres installed - simply copy and paste the script into an empty directory.

It starts out with Puma. If you open two browser windows and press refresh a visitor counter will update in both. Simple stuff.

Next, run:

bundle remove puma
bundle add falcon

And restart the server and the websocket fails to start.

In order to debug this, I repeated this with the latest Rails main, which I instrumented with log statements. It seems that websocket_possible? is nil at the following line https://github.com/rails/rails/blob/355fd5929058ee3a73032c5f62de3d8819550482/actioncable/lib/action_cable/connection/base.rb#L75; this in turn is because ::WebSocket::Driver.websocket?(env) is false at https://github.com/rails/rails/blob/355fd5929058ee3a73032c5f62de3d8819550482/actioncable/lib/action_cable/connection/web_socket.rb#L10

ioquatix commented 1 year ago

I will take a look, thanks for the detailed information.

tinco commented 1 year ago

Hi! I just ran into this issue as well. If there's an obvious step, I also missed it, I get the same error.

Kind of weird that ::WebSocket::Driver.websocket? would return false, as it's super generic and not changed for 9 years:

    def self.websocket?(env)
      connection = env['HTTP_CONNECTION'] || ''
      upgrade    = env['HTTP_UPGRADE']    || ''

      env['REQUEST_METHOD'] == 'GET' and
      connection.downcase.split(/ *, */).include?('upgrade') and
      upgrade.downcase == 'websocket'
    end

Does env get mangled somehow?

tinco commented 1 year ago

Oh actually there's a hint in the logged message:

Failed to upgrade to WebSocket (REQUEST_METHOD: GET, HTTP_CONNECTION: keep-alive, Upgrade, HTTP_UPGRADE: )

Note how the HTTP_UPGRADE is empty, and instead there's an Upgrade in HTTP_CONNECTION instead..

edit: Nvm, upgrade should to be there. But in the HTTP_UPGRADE there's supposed to be a websocket string as well.

buhrmi commented 10 months ago

also running into this now. anyone managed to find out what's happening?

Amnesthesia commented 7 months ago

11:56:29 E ActionCable -- Failed to upgrade to WebSocket (REQUEST_METHOD: GET, HTTP_CONNECTION: Upgrade, HTTP_UPGRADE: ) we're running into this too, we can't really use Falcon without nginx because of this

ioquatix commented 7 months ago

Thanks for the reminder, I'll take a look.

ioquatix commented 7 months ago

I believe this is a similar issue to https://github.com/socketry/async-websocket/issues/55

If possible, can someone test adding the following middleware to see if it resolves the issue:

class RackProtocolToConnectionUpgrade
  def initialize(app)
    @app = app
  end

  def call(env)
    if protocol = env['rack.protocol']
      env['HTTP_UPGRADE'] = protocol
      # Maybe need to modify connection header too.
    end

    @app.call(env)
  end
end

This won't work for HTTP/2 however...

Pash-tet commented 7 months ago

I have same problem, action cable not working on rails 7.1.2 with falcon. Tried to use middleware - same result, with middleware https://github.com/rails/rails/blob/355fd5929058ee3a73032c5f62de3d8819550482/actioncable/lib/action_cable/connection/stream.rb#L97C56-L97C56 @socket_object.env["rack.hijack"] return nil

ioquatix commented 7 months ago

Thanks for the reports, after thinking about this problem, I'll try to address it directly in protocol-rack with a compatibility shim.

ioquatix commented 7 months ago

Apologies, I've realised the issue here. Full hijack is not supported by protocol-rack. I'll look into potential solutions. In the past it was supported, but it's replaced by full-bidirectional streaming in Rack 3 and that's what falcon provides out of the box since it's a better abstraction. I'll consider if there is an easy way to make this work with ActionCable.

ioquatix commented 6 months ago

After playing around with it, it wasn't so hard to restore full hijack support. This should work. However, I don't think full hijack is a good long term solution for Falcon. Can you please try protocol-rack v0.4+ and it should work.

Pash-tet commented 6 months ago

After playing around with it, it wasn't so hard to restore full hijack support. This should work. However, I don't think full hijack is a good long term solution for Falcon. Can you please try protocol-rack v0.4+ and it should work.

Hello, i tried with protocol-rack v0.4+.

First i got error on this line https://github.com/socketry/protocol-rack/blob/3a9ff0ef95198c3e4070c723ce54bc63d7dd4074/lib/protocol/rack/adapter/generic.rb#L77 NameError: uninitialized constant Protocol::Rack::Adapter::Generic::RACK_HIJACK. I rewrite it to Protocol::Rack::Adapter::Rack2::RACK_HIJACK and this error is gone.

Then i got error in middleware env['HTTP_UPGRADE'] = protocol, change to env['HTTP_UPGRADE'] = protocol.first, this fix second error.

Then i got this error, and i stuck on it (

warn: Async::Task: Reading HTTP/1.1 requests for Async::HTTP::Protocol::HTTP1::Server. [oid=0x4a10] [ec=0x3a5c] [pid=12191] [2023-12-11 10:12:59 +0300]
               | Task may have ended with unhandled exception.
               |   NoMethodError: undefined method `write' for nil:NilClass
               |   → .rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/protocol-http1-0.16.0/lib/protocol/http1/connection.rb:126 in `write_response'
               |     .rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/async-http-0.61.0/lib/async/http/protocol/http1/server.rb:63 in `each'
               |     .rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/async-http-0.61.0/lib/async/http/server.rb:40 in `accept'
               |     .rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/async-io-1.38.0/lib/async/io/server.rb:15 in `block in accept_each'
               |     .rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/async-io-1.38.0/lib/async/io/socket.rb:58 in `block in accept'
               |     .rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/async-2.6.5/lib/async/task.rb:160 in `block in run'
               |     .rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/async-2.6.5/lib/async/task.rb:330 in `block in schedule'
ioquatix commented 6 months ago

Just to confirm, you are using Rack 3+ right?

Oh, also, that "warning" you can ignore for now, it's because the connection got hijacked and it can't write a normal response, it's noisy but shouldn't impact the websocket.

ioquatix commented 6 months ago

Ohhhh it looks like I put the constant in the wrong place... hold on, let me fix that.

ioquatix commented 6 months ago

Ah, you no longer need the middleware proposed on this issue either, so you can remove that completely.

ioquatix commented 6 months ago

I released v0.4.1 which has the fixes discussed above, except the server will still print the warning which you can ignore.

Pash-tet commented 6 months ago

I released v0.4.1 which has the fixes discussed above, except the server will still print the warning which you can ignore.

Thank you ! Now it work like a charm ❤️ I will test further

buhrmi commented 6 months ago

Yooo just tuning in to say that adding protocol-rack 0.4.1 to my Gemfile seems to have solved the issue. supercool 👍

ioquatix commented 6 months ago

Thanks for the reports of success. As such, I'll close this issue. Feel free to open a new issue if you find other problems.