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

Fix Net::HTTP nil body bug #3825

Merged
merged 1 commit into from May 18, 2016

Conversation

Projects
None yet
5 participants
@chrisberkhout

chrisberkhout commented Apr 25, 2016

No description provided.

@kares kares added this to the JRuby 1.7.26 milestone Apr 25, 2016

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Apr 28, 2016

Member

Huh, I can't even find this code in current net/http.rb.

Member

headius commented Apr 28, 2016

Huh, I can't even find this code in current net/http.rb.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Apr 28, 2016

Member

It appears based on ruby/ruby@b1a0509 this functionality was moved to response.rb and additional guards were added to ensure empty bodies don't get decompressed. If you can reproduce this, it would be good to know if a Ruby 2.2-compatible runtime passes your case.

Member

headius commented Apr 28, 2016

It appears based on ruby/ruby@b1a0509 this functionality was moved to response.rb and additional guards were added to ensure empty bodies don't get decompressed. If you can reproduce this, it would be good to know if a Ruby 2.2-compatible runtime passes your case.

@chrisberkhout

This comment has been minimized.

Show comment
Hide comment
@chrisberkhout

chrisberkhout Apr 28, 2016

@headius The problem is not in the JRuby 9k series, only in JRuby 1.x (including the latest 1.7.25), using the 1.9 stdlib (later stdlib versions do fix it).

I noticed there are multiple versions of the stdlib in the 1.7.25 code. My install used the 1.9 versions, and I'm not sure how the others are used (looked for documentation and command line options, but didn't find any).

My intention with the PR is that the upcoming 1.7.26 release doesn't have this no method on nil issue. I asked on IRC and understood that the PR should merge into the branch jruby-1_7 (as this one requests) to be in the next 1.x release.

chrisberkhout commented Apr 28, 2016

@headius The problem is not in the JRuby 9k series, only in JRuby 1.x (including the latest 1.7.25), using the 1.9 stdlib (later stdlib versions do fix it).

I noticed there are multiple versions of the stdlib in the 1.7.25 code. My install used the 1.9 versions, and I'm not sure how the others are used (looked for documentation and command line options, but didn't find any).

My intention with the PR is that the upcoming 1.7.26 release doesn't have this no method on nil issue. I asked on IRC and understood that the PR should merge into the branch jruby-1_7 (as this one requests) to be in the next 1.x release.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Apr 28, 2016

Member

@chrisberkhout Thank you for clarifying about 9k!

The 1.8 and 2.0 stdlibs in JRuby 1.7.x are used when running with --1.8 or --2.0 modes. The 2.0 stdlib probably doesn't have its own copy of net/http. We could fix 1.8, but it's a pretty low priority.

Thanks for the PR!

Member

headius commented Apr 28, 2016

@chrisberkhout Thank you for clarifying about 9k!

The 1.8 and 2.0 stdlibs in JRuby 1.7.x are used when running with --1.8 or --2.0 modes. The 2.0 stdlib probably doesn't have its own copy of net/http. We could fix 1.8, but it's a pretty low priority.

Thanks for the PR!

@chrisberkhout

This comment has been minimized.

Show comment
Hide comment
@chrisberkhout

chrisberkhout Apr 28, 2016

@headius No worries.

I checked again, and the weird response we had is correctly processed by 1.8 or 2.0. So that's it! :)

chrisberkhout commented Apr 28, 2016

@headius No worries.

I checked again, and the weird response we had is correctly processed by 1.8 or 2.0. So that's it! :)

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 2, 2016

Member

@chrisberkhout Thanks for confirming!

Member

headius commented May 2, 2016

@chrisberkhout Thanks for confirming!

@BanzaiMan BanzaiMan closed this May 17, 2016

@chrisberkhout

This comment has been minimized.

Show comment
Hide comment
@chrisberkhout

chrisberkhout May 18, 2016

@BanzaiMan I'm wondering, why wasn't this merged?

chrisberkhout commented May 18, 2016

@BanzaiMan I'm wondering, why wasn't this merged?

@enebo enebo reopened this May 18, 2016

@enebo enebo merged commit 5b83abb into jruby:jruby-1_7 May 18, 2016

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor was unable to build non-mergeable pull request
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo May 18, 2016

Member

@chrisberkhout I think @BanzaiMan thought it had already been merged.

Member

enebo commented May 18, 2016

@chrisberkhout I think @BanzaiMan thought it had already been merged.

@chrisberkhout

This comment has been minimized.

Show comment
Hide comment
@chrisberkhout

chrisberkhout commented May 19, 2016

Cool. Thanks @enebo!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment