Net::HTTP ignores HTTP_PROXY and http.proxyHost #195

Merged
merged 1 commit into from Aug 28, 2012

Projects

None yet

3 participants

@dekz

Addresses open issue #134, JRUBY-6701. Allowing the JVM arguments 'http.proxyHost' and 'http.proxyPort' to be set and used by Net::HTTP.

@nahi
JRuby Team member

It would be better setting env var http_proxy according to JVM arg for open-uri and rubygems.
CRuby's net/http ignores OS's env setting (http_proxy/HTTP_PROXY) so JVM arg should be ignored as the same.

You may want to file a ticket to bugs.ruby-lang.org for asking net/http to honor OS's proxy setting.

@dekz

Thanks nahi, I will open a bug on ruby-lang to address the ENV http_proxy. I believe this instance is a special case where JRuby ignores a Java defined argument, not an ENV setting. Since it is a Java defined setting it should be read and used (and where ruby-lang doesn't have precedence), whereas the ENV http_proxy setting is debatable. Thoughts?

EDIT:

I think I understand your point a little more now. I wasn't aware that most of the JRuby ruby code is the exact same as ruby-langs. So you're saying since ruby-lang explicitly ignores all 'environment' settings, so should JRuby (where environment is also the JVM)?

@headius
JRuby Team member

I disagree with @nahi here. The JSE proxy setting is a JVM runtime behavior anyone running on JVM should probably be aware of. Even further, those who do rely on it like @dekz see the very surprising behavior of JRuby ignoring JSE proxy.

I do agree that it's perhaps a little surprising that there would be environment settings that would affect JRuby and not MRI, but then again JRuby runs on a different runtime...differences are part of the story.

@nahi
JRuby Team member

@dekz Yes, that's my thinking.

@headius We might be able to customize and maintain net/http, net/ftp, open-uri and rubygems (really?) but we cannot support all other non-standard HTTP clients like eventmachine, rest-client, curb and httpclient. I think it's better to refrain from customizing stdlibs. Converting JSE proxy setting to VM local ENV would work, isn't it enough?

@headius
JRuby Team member

@nahi If the maintenance load is too much, then we definitely would want to find an alternative. I don't know how the JSE proxy settings propagate into normal Java APIs, but it sounds like they aren't automatically just "on" so there's manual work we would need to do.

Your idea of just turning those properties into ENV values is interesting, but my concern is propagating that ENV to subprocesses that would normally not see it. @dekz patch is also not too invasive, but I have no idea how many other places would need to be patched to support JSE proxy consistently.

Another option would be a library you can require that makes net/* honor JSE/JVM proxy settings. In other words, make it opt in without the user having to modify every piece of code that calls into net/* to use those settings...with @dekz (monkey) patch behind a JRuby-specific library.

For now I'll defer to you (@nahi) to decide how to approach this (if we approach it at all).

@dekz

I can see why this patch can be both good and bad. Is there precedence where JSE settings are applied to the runtime which may in effect change the output when compared with MRI? If not this will add precedence to all future issues whether the JSE setting should be applied.

As @nahi has said it does impact a number of higher level functionality such as open-uri which could cause additional maintenance, tests and burden on other software (such as HTTP clients RestClient et al).

I don't believe simply propagating the JSE setting to the ENV is enough or correct in this situation.

I think the question to answer is, Should we always implement the expected functionality of JSE settings and trend away from MRI, defining a different platform.

At the end of the day, if the HTTP functionality was written to use Java Internals it should propagate the setting. Simply because we are using the MRI implementation of HTTP it does not. So arguably we should take this setting into account.

@headius
JRuby Team member

Ok, I'm going to merge this in some way or another, but there are a couple concerns:

  • If JSE proxy setting is on, there's no way to make this code ignore it other than by clearing the property or by passing a different proxy. Does anyone foresee that as a problem?

  • The patch needs to use ENV_JAVA so there's no hard dependency on the 'java' library in JRuby. ENV_JAVA mirrors Java system properties.

For the moment I'm going to go ahead and just use the proxy properties unconditionally and we'll see how it goes.

@headius headius merged commit e283bc9 into jruby:master Aug 28, 2012
@headius headius added a commit that referenced this pull request Aug 28, 2012
@headius headius Tweaks for #195:
* Use ENV_JAVA so there's no dependency on 'java' library.
* Add comment explaining our defaulting to JSE proxy properties
* Make the same change for 1.8's net/http.
d2c55d2
@dekz

Thanks headius, the only thing I am a little worried about is how well this works with open-uri. But if all tests seem fine, it's worth the merge.

If JSE proxy setting is on, there's no way to make this code ignore it other than by clearing the property or by passing a different proxy. Does anyone foresee that as a problem?

This seems to be the way it is designed to work on the JVM, so I don't see a problem here.

The patch needs to use ENV_JAVA so there's no hard dependency on the 'java' library in JRuby. ENV_JAVA mirrors Java system properties.

Sounds much cleaner.

@eregon eregon added a commit that referenced this pull request Mar 1, 2016
@eregon eregon Squashed 'spec/ruby/' changes from d9a07bf..8d632d3
8d632d3 Fix a typo in Enumerable#grep specs
eb9a915 Clean up the entire spec temp dir when finished with mock dirs.
6793fc5 Add specs for constants with op assigns (2.0+ feature)
2428acb Duplicate grep_v block specs for grep.
f8fe203 Fix calls to close_read mistakenly copied as close_write.
6c483a1 Remove trailing spaces
aafde9a Setting umask seems unneeded for Dir specs
75c3c99 Fix indent and verify better the behavior of Dir.chdir without arguments
cafd965 Use full names in singleton_method_* fixtures to avoid confusion
793b0f5 Improve specs of Module#method_added and BasicObject#singleton_method_added
8c780f2 Fix namespace of BasicObject fixtures
19b3530 Unshare BasicObject#singleton_method_{added,removed,undefined}
7ec6446 Use ScratchPad instead of a global variable in Module#method_added spec
55bbe8d Merge pull request #199 from wied03/master
103e25a Test more block scenarios with super
2e3116d Remove unnecessary version guards
8d4cadb Prefer duplication to conditional code in specs
bb36c64 Merge pull request #197 from nobu/Numeric#step-error
33b90eb Numeric#step now raises TypeError
5940ab5 Fix lower version
dfb4e4b Merge pull request #196 from nobu/Numeric#step-error
bad02c6 Numeric#step will not raise ArgumentError
c559cd1 Fix location of version guard in Marshal#load
dc20179 Merge pull request #193 from unak/patch-1
51da767 Merge pull request #195 from iliabylich/reset-dollar-comma-gvar-back-to-nil
ef2bd56 Reset $, global variable back to nil to prevent IO.print from breaking.
44c6e32 taints float is 2.2 spec.
73e1e0b Specify Array#dig with non-numeric index
af15432 Merge pull request #191 from ruby/vais/masgn-const
86e0894 Wrap specs for multiple assignment to constants in a VariableSpecs module
fc1fee4 Add an example describing the behavior succinctly and a possible implementation for Fixnum#[]
e7b8c65 Merge pull request #192 from mame/fixnum-aref-negative
2900958 Merge pull request #190 from nobu/Symbol#match-fix
7604cbc check if the result is a MatchData
03f6140 Add some specs for `Fixnum#[]` when self is negative
4f8ca98 Add specs for multiple assignment to constants
51d46b1 New examples of Symbol#match since 2.4
7c797ee Merge pull request #189 from nobu/Symbol#match-fix
e450f73 Symbol#match will change at 2.4
e5db1f0 Merge pull request #187 from alex88/patch-1
0a9725a Add tests for Time.at with BigDecimal input

git-subtree-dir: spec/ruby
git-subtree-split: 8d632d36026879e617eae21ba913873e72a70dda
af839a0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment