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

8262442: (windows) Use all proxy configuration sources when java.net.useSystemProxies=true #5995

Closed
wants to merge 2 commits into from

Conversation

djelinski
Copy link
Member

@djelinski djelinski commented Oct 18, 2021

With this patch DefaultProxySelector first attempts to use proxy config autodetection (http://wpad/wpad.dat) when that is configured and available.
If proxy config autodetection is unavailable, selector tries to use configured proxy script (again, if configured and available)
If both the above options fail, selector uses the configured proxy.

Verified on Windows 10 that:

  • when fAutoDetect is true, http://wpad/wpad.dat refers to an existing file, the file has correct syntax and returns a proxy, that proxy is used
  • when fAutoDetect is true, but the autoconfig file is not available / unusable for any reason, selector fails over to the next configured method
  • when lpszAutoConfigUrl is set and usable, the proxy returned is used
  • when lpszAutoConfigUrl is not set or unusable, selector fails over to next method
  • when lpszProxy is configured, that proxy is used
  • otherwise selector uses direct connection

The proxy configuration scripts are cached on system level, so testing (alternating between good and broken autoconfig script) may require waiting for the caches to invalidate.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8262442: (windows) Use all proxy configuration sources when java.net.useSystemProxies=true

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5995/head:pull/5995
$ git checkout pull/5995

Update a local copy of the PR:
$ git checkout pull/5995
$ git pull https://git.openjdk.java.net/jdk pull/5995/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5995

View PR using the GUI difftool:
$ git pr show -t 5995

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5995.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 18, 2021

👋 Welcome back djelinski! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label Oct 18, 2021
@openjdk
Copy link

openjdk bot commented Oct 18, 2021

@djelinski The following label will be automatically applied to this pull request:

  • net

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the net net-dev@openjdk.org label Oct 18, 2021
@mlbridge
Copy link

mlbridge bot commented Oct 18, 2021

Webrevs

@djelinski
Copy link
Member Author

No automatic tests; to test this manually I used 2 proxy autoconfig files of form:

function FindProxyForURL(url, host) {
  if (url.includes("example")) {
    return "PROXY 10.4.5.6:8192";
  } else {
    return "DIRECT";
  }
}

one named wpad.dat, other named proxy.pac, returning different proxy addresses so that I could figure out which one was used. These files were served by a http server running on localhost, conveniently set up with python -m http.server 80.
Next, I added the following line to c:\Windows\System32\drivers\etc\hosts:

127.0.0.1 wpad

Next, I used the little Java program found in JDK-8262442 to test what ProxySelector returned with different combinations of proxy configuration options.

@djelinski
Copy link
Member Author

Any comments on this one?

@dfuch
Copy link
Member

dfuch commented Nov 29, 2021

See my comment on #6090 (comment)
Again - this looks like a good fix - but we would feel more comfortable integrating it in the early development phase of JDK 19, in a few days...

@bridgekeeper
Copy link

bridgekeeper bot commented Dec 27, 2021

@djelinski This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@djelinski
Copy link
Member Author

bump

@dfuch
Copy link
Member

dfuch commented Jan 11, 2022

Hi Daniel - could you update the second copyright year in the file header to 2022?
Thanks!

@djelinski
Copy link
Member Author

Thanks!

/integrate

@openjdk
Copy link

openjdk bot commented Jan 11, 2022

@djelinski This pull request has not yet been marked as ready for integration.

@djelinski djelinski changed the title 8262442 (windows) use all proxy configuration sources when java.net.useSystemProxies=true 8262442: (windows) Use all proxy configuration sources when java.net.useSystemProxies=true Jan 12, 2022
@openjdk
Copy link

openjdk bot commented Jan 12, 2022

@djelinski This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8262442: (windows) Use all proxy configuration sources when java.net.useSystemProxies=true

Reviewed-by: dfuchs

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 1301 new commits pushed to the master branch:

  • 319d230: 8277463: JFileChooser with Metal L&F doesn't show non-canonical UNC path in - Look in
  • 13bfb49: 6496103: isFileHidingEnabled return false by default
  • f16f6a9: 8279821: JFR: Log warnings properly when loading a misconfigured .jfc file
  • 1c688f4: 8279900: compiler/vectorization/TestPopCountVectorLong.java fails due to vpopcntdq is not supported
  • 3aaa098: 8276694: Pattern trailing unescaped backslash causes internal error
  • 36f41cb: 8279884: Use better file for cygwin source permission check
  • c4518e2: 8278868: Add x86 vectorization support for Long.bitCount()
  • 6714184: 8279700: Parallel: Simplify ScavengeRootsTask constructor API
  • cfee451: 8273914: Indy string concat changes order of operations
  • c3d0a94: 8279833: Loop optimization issue in String.encodeUTF8_UTF16
  • ... and 1291 more: https://git.openjdk.java.net/jdk/compare/56b8b35286634f2d2224ca445bc9f32ff284ae74...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@dfuch) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jan 12, 2022
@djelinski
Copy link
Member Author

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Jan 12, 2022
@openjdk
Copy link

openjdk bot commented Jan 12, 2022

@djelinski
Your change (at version f4d3bad) is now ready to be sponsored by a Committer.

@dfuch
Copy link
Member

dfuch commented Jan 13, 2022

/sponsor

@openjdk
Copy link

openjdk bot commented Jan 13, 2022

Going to push as commit 6fcaa32.
Since your change was applied there have been 1323 commits pushed to the master branch:

  • c17a012: 8278961: Enable debug logging in java/net/DatagramSocket/SendDatagramToBadAddress.java
  • b61a4af: 8259774: Deprecate -XX:FlightRecorderOptions:samplethreads
  • 6933934: 8278597: Remove outdated comments regarding RMISecurityManager in HotSpotAgent.java
  • 4851948: 8279903: Redundant modulo operation in ECDHKeyAgreement
  • 67e3d51: Merge
  • 5aecb37: 8206181: ExceptionInInitializerError: improve handling of exceptions in user-provided taglets
  • 86d0abb: 8279695: [TESTBUG] modify compiler/loopopts/TestSkeletonPredicateNegation.java to run on C1 also
  • 6d7db4b: 8279356: Method linking fails with guarantee(mh->adapter() != NULL) failed: Adapter blob must already exist!
  • 92307e5: 8278489: Preserve result in native wrapper with +UseHeavyMonitors
  • bbc1ddb: 8278267: ARM32: several vector test failures for ASHR
  • ... and 1313 more: https://git.openjdk.java.net/jdk/compare/56b8b35286634f2d2224ca445bc9f32ff284ae74...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Jan 13, 2022
@openjdk openjdk bot closed this Jan 13, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Jan 13, 2022
@openjdk
Copy link

openjdk bot commented Jan 13, 2022

@dfuch @djelinski Pushed as commit 6fcaa32.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@djelinski djelinski deleted the use-all-proxy-configs branch January 13, 2022 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated net net-dev@openjdk.org
Development

Successfully merging this pull request may close these issues.

2 participants