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

7146776: deadlock between URLStreamHandler.getHostAddress and file.Handler.openconnection #2109

Closed
wants to merge 2 commits into from

Conversation

jaikiran
Copy link
Member

@jaikiran jaikiran commented Jan 16, 2021

As noted by the reporter in the comments of https://bugs.openjdk.java.net/browse/JDK-7146776, the URLStreamHandler in its getHostAddress is incorrectly synchronizing on the URLStreamHandler instance, instead of synchronizing on the URL instance that is being passed to that method.

The implementation of URLStreamHandler#getHostAddress checks for the (package private) hostAddress member of the passed URL and if it isn't set then does a DNS lookup to get the address and finally sets the hostAddress member of the URL to this returned address. The access to hostAddress of URL needs to be synchronized since it can be updated (in a different thread) in the URL#set methods.

The commit here removes the synchronization from the URLStreamHandler#getHostAddress and moves the entire implementation into a new (package private) URL#getHostAddress method and then calls this new method from the URLStreamHandler#getHostAddress. I decided to do it this way instead of just leaving the implementation in URLStreamHandler#getHostAddress and synchronizing on the passed URL instance, since IMO, this makes it much more easier to see (in one place within the URL class) what the synchronization requirements are when dealing with the hostAddress. Furthermore, this now allows the hostAddress to be made a private member (which I have done in this commit) instead of the previous package private access. I checked that no other code tries to access this package private URL#hostAddress.

I can't think of an easy way to add a jtreg test for this change, so I haven't added one.


Progress

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

Issue

  • JDK-7146776: deadlock between URLStreamHandler.getHostAddress and file.Handler.openconnection

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2109/head:pull/2109
$ git checkout pull/2109

@bridgekeeper
Copy link

bridgekeeper bot commented Jan 16, 2021

👋 Welcome back jpai! 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 Jan 16, 2021
@openjdk
Copy link

openjdk bot commented Jan 16, 2021

@jaikiran 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 Jan 16, 2021
@mlbridge
Copy link

mlbridge bot commented Jan 16, 2021

Webrevs

* A {@link SecurityException} or an {@link UnknownHostException}
* while getting the host address will result in this method returning
* {@code null}
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix looks okay but the javadoc might be a big clearer if you move the text into the method description.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello Alan, I've now updated the PR to move this text into the method description.

@openjdk
Copy link

openjdk bot commented Jan 17, 2021

@jaikiran 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:

7146776: deadlock between URLStreamHandler.getHostAddress and file.Handler.openconnection

Reviewed-by: alanb, chegar

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 9 new commits pushed to the master branch:

  • e93f08e: 8074101: Add verification that all tasks are actually claimed during roots processing
  • 917f7e9: 8259650: javax/swing/JComponent/7154030/bug7154030.java still fails with "Exception: Failed to hide opaque button"
  • 3f19ef6: 8202880: Test javax/swing/JPopupMenu/8075063/ContextMenuScrollTest.java fails
  • 68cf65d: 8023980: JCE doesn't provide any class to handle RSA private key in PKCS#1
  • 5dc5d94: 8256110: Create implementation for NSAccessibilityStepper protocol
  • 5f2e280: 8259865: (fs) test/jdk/java/nio/file/attribute/UserDefinedFileAttributeView/Basic.java failing on macOS 10.13
  • da4cf05: 8258755: jpackage: Invalid 32-bit exe when building app-image
  • c3bdbf9: 8259238: Clean up Log.java and remove usage of non-final static variables.
  • 6d6a23e: 8259062: Remove MacAppStoreBundler

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jan 17, 2021
Copy link
Member

@ChrisHegarty ChrisHegarty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@jaikiran
Copy link
Member Author

/integrate

@openjdk openjdk bot closed this Jan 18, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 18, 2021
@openjdk
Copy link

openjdk bot commented Jan 18, 2021

@jaikiran Since your change was applied there have been 11 commits pushed to the master branch:

  • 61292be: 8259681: Remove the Marlin rendering engine (single-precision)
  • ff275b3: 8259403: Zero: crash with NULL MethodHandle receiver
  • e93f08e: 8074101: Add verification that all tasks are actually claimed during roots processing
  • 917f7e9: 8259650: javax/swing/JComponent/7154030/bug7154030.java still fails with "Exception: Failed to hide opaque button"
  • 3f19ef6: 8202880: Test javax/swing/JPopupMenu/8075063/ContextMenuScrollTest.java fails
  • 68cf65d: 8023980: JCE doesn't provide any class to handle RSA private key in PKCS#1
  • 5dc5d94: 8256110: Create implementation for NSAccessibilityStepper protocol
  • 5f2e280: 8259865: (fs) test/jdk/java/nio/file/attribute/UserDefinedFileAttributeView/Basic.java failing on macOS 10.13
  • da4cf05: 8258755: jpackage: Invalid 32-bit exe when building app-image
  • c3bdbf9: 8259238: Clean up Log.java and remove usage of non-final static variables.
  • ... and 1 more: https://git.openjdk.java.net/jdk/compare/afd3f78aec8a67bbfba291d0b94c2779490b945f...master

Your commit was automatically rebased without conflicts.

Pushed as commit db9c114.

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

Copy link
Member

@dfuch dfuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Thanks Jaikiran!

@jaikiran jaikiran deleted the 7146776 branch January 25, 2024 09:39
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
4 participants