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

Don't delete content-length before yielding inflate body #50

Closed
wants to merge 1 commit into from

Conversation

st0012
Copy link
Member

@st0012 st0012 commented Apr 2, 2022

Fixes #49

result when running the same repro script

❯ time ruby -Ilib -rnet/http -rdebug -e "puts Net::HTTP.get(URI.parse('https://raw.githubusercontent.com/ruby/debug/master/.gitignore'))"
/.bundle/
/.yardoc
/_yardoc/
/coverage/
/doc/
/pkg/
/spec/reports/
/tmp/
*.bundle
/Gemfile.lock
/lib/debug/debug.so
.ruby-version
/debugAdapterProtocol.json
/chromeDevToolsProtocol.json
ruby -Ilib -rnet/http -rdebug -e   0.24s user 0.10s system 57% cpu 0.606 total

The content-length information is still needed when evaluating the given block. Removing it before that will make read_body_0 switch from @socket.read length to @socket.read_all, which can take very long and cause timeout.

I'm not sure how to reproduce the issue with tests though.

The content-length information is still needed when evaluating the given
block. Removing it will make `read_body_0` switch from `@socket.read
length` to `@socket.read_all`, which can take very long and cause
timeout.
@st0012
Copy link
Member Author

st0012 commented Apr 2, 2022

@jeremyevans I'm not familiar with the net-http's implementation so this may not be the best solution. If that's the case, feel free to close it 🙂

@jeremyevans
Copy link
Contributor

I've tested this this fixes Ruby CI, and incorporated the fix in #51. Thanks @st0012!

@jeremyevans jeremyevans closed this Apr 4, 2022
@st0012 st0012 deleted the fix-#49 branch April 4, 2022 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Latest content-length change makes it extremely long to get response
2 participants