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

Improve handling of unconsumed request bodies at end of http1 requests #138

Merged
merged 1 commit into from
Oct 24, 2023

Conversation

zarqman
Copy link
Contributor

@zarqman zarqman commented Sep 19, 2023

This PR includes 2 related items:

  1. Fixes an old regression where request = nil should be conditional upon the request body, but was incorrectly the response body. Looking at the git history, the original behavior was ... unless request.body and was changed in b3cc9ce, seemingly accidentally during a refactor.

    I occasionally experience random hangs when running my test suite and believe this might be one cause of them.

  2. Changes the handling of unconsumed bodies from request.finish to request.each{}.

    finish buffers the request fully in memory as a Buffered, causing needless memory bloat in the ruby process. Using each skips the buffering, but is otherwise behaves the same.

Types of Changes

  • Bug fix.

Contribution

Sorry, something went wrong.

@zarqman zarqman force-pushed the request-body-unfinished branch from b86d8c0 to f052feb Compare September 19, 2023 22:33
@zarqman
Copy link
Contributor Author

zarqman commented Sep 19, 2023

Revised to remove the previous attempt at request.close as further testing shows it might be causing a regression. Instead, using request.each{} which functions like the original call to request.finish except that it avoids buffering the request body into memory.

@ioquatix ioquatix force-pushed the request-body-unfinished branch from f052feb to 9f325c7 Compare October 24, 2023 12:14
@ioquatix
Copy link
Member

This seems reasonable to me, thanks for investigating the history.

@ioquatix ioquatix merged commit fe9fcc4 into socketry:main Oct 24, 2023
15 of 18 checks passed
@zarqman zarqman deleted the request-body-unfinished branch October 26, 2023 22:24
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

2 participants