Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support to keep open streaming connections with Puma #1858

Merged
merged 2 commits into from
Feb 10, 2023

Conversation

jkowens
Copy link
Member

@jkowens jkowens commented Jan 2, 2023

@ioquatix I also tried to test this with Falcon as well, but it didn't seem to work. Does Falcon buffer the response?

This is somewhat modeled after streaming in Roda: https://github.com/jeremyevans/roda/blob/c919ac9c7e88f159f8c6185b0bdb5a9311e24d93/lib/roda/plugins/streaming.rb#L143

@ioquatix
Copy link
Contributor

ioquatix commented Jan 2, 2023

Falcon doesn't buffer the response body. Are you using Rack 3? Can you print out the middleware stack? Can you run with falcon verbose mode to see how the output body is being handled?

@jkowens
Copy link
Member Author

jkowens commented Jan 2, 2023

@ioquatix ah I think I see the issue. It looks like request that establishes the stream (which creates an infinite loop to hold the connection) prevents the server from handling anymore requests.

@dentarg
Copy link
Member

dentarg commented Jan 2, 2023

This PR is not using Rack 3 @ioquatix (I'm working on it in #1857) – if we're talking Rack 3, would it be easier to get our tests and example working with Puma/Falcon if we adopted "callable body" (rack/rack#1745, puma/puma#2740)?

I need to play with this myself, and catch up on everything discussed, rack/rack#1600 also has a fair bit of discussion, still have a big bunch unread emails from rack/rack from last year and summer 2021 😅

@ioquatix
Copy link
Contributor

ioquatix commented Jan 2, 2023

@dentarg you should consult the upgrade guide if you want to see a summary of all changes that affect applications/middlewares): https://github.com/rack/rack/blob/main/UPGRADE-GUIDE.md

Yes you should rely on callable bodies as this is the correct way to implement streaming with Rack. Everything else is a hack and may have unintended buffering.

when 'HTTP'
'net/http/server'
when 'puma'
'puma/rack/handler'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests output show puma is not installed, skipping integration tests so I think this require is not working as expected.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh good catch 😬 Explains why things went so smooth 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants