search

socketry / async-http

MIT License
320 stars 46 forks source link

Connections hijacked by ActionCable raise an error #150

Closed yard closed 7 months ago

yard commented 8 months ago

When a connection gets hijacked by ActionCable, the HTTP1 server crashes the task – this seems to be a somewhat-known issue, has been mentioned in multiple places and indeed seems not-so-critical.

The culprit seems to be this line: https://github.com/socketry/async-http/blob/main/lib/async/http/protocol/http1/server.rb#L53. @stream is nil, by body is not (it's actually an instance of Protocol::Rack::Body::Enumerable). Looks like we can simply check for #empty? method on the body and get rid of the error.

I am happy to create a PR and submit it here, but before doing so, wanted to check if that is indeed a valid fix? Any implications of not sending the response when the body is empty and the @stream is nil? Or there is a case where this situation is valid and the fix belongs to a different place?

ioquatix commented 8 months ago

Yes, I'm okay with the proposed PR. I may have additional feedback when I review it, but you are correct, that is the place to deal with it.

yard commented 8 months ago

Thank you @ioquatix, I have opened a https://github.com/socketry/async-http/pull/151 that deals with it. Happy to iterate on it as per your feedback

ioquatix commented 7 months ago

This should be fixed now, thanks to your effort. Thanks!