8305763 : Parsing a URI with an underscore goes through a silent exception, negatively impacting performance

[dhanalla]

Issue 8305763 : Using underscores in the name for a URI triggers a silent exception in the java standard library, which consumes 5% of the CPU.

Exception:
java.net.URISyntaxException: Illegal character in hostname at index N: xyz1_abcd.com
at java.base/java.net.URI$Parser.fail(URI.java:2943)
at java.base/java.net.URI$Parser.parseHostname(URI.java:3487)
at java.base/java.net.URI$Parser.parseServer(URI.java:3329)

This exception is silent and does not produce any messages, except for ODP profiler, there is no other evidence that it’s happening (the stack trace above was printed after changes to Java library). The reason for this is because of how the URI creation is implemented in the java.net.URI class. There are two paths for creating a valid URI, and one of them goes through an exception.

We can see that if parseServer fails, there is still a way the authority gets assigned and we don’t throw an exception from the method. This means, not being able to parse the server is ok and the exception is silenced. In our case, the server parsing fails because we find an illegal character, as only alphanumeric and dash characters are allowed.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8305763: Parsing a URI with an underscore goes through a silent exception, negatively impacting performance

Reviewers

• Daniel Fuchs (@dfuch - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/13430/head:pull/13430
$ git checkout pull/13430

Update a local copy of the PR:
$ git checkout pull/13430
$ git pull https://git.openjdk.org/jdk.git pull/13430/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13430

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/13430.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back dhanalla! 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]

@dhanalla 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.

[mlbridge]

Webrevs

• 03: Full - Incremental (feeb2a82)
• 02: Full - Incremental (1e7b597d)
• 01: Full - Incremental (00fa09ea)
• 00: Full (3353228b)

[dfuch]

From a quick look at the proposed change, I got the feeling that this change might not be appropriate: I suspect it will let host be assigned to the reg_name.
We want to preserve the long standing behavior that:

jshell> new URI("http://foo_bar:8080/").getHost()
$1 ==> null

Is this still the case after your proposed changes?

[dhanalla]

Thanks for reviewing @dfuch,
With this change, the below operation returns hostname "foo_bar"

jshell> new URI("http://foo_bar:8080/").getHost()

I will work on addressing this to keep the old behavior unchanged.

[dfuch]

I will work on addressing this to keep the old behavior unchanged.

Please make sure to run the java/net/URI tests - and when these are successful, tier1, tier2 and tier3.

[dhanalla]

Thanks @dfuch,
I have validated the following tests with the new change.

1. JShell:
   jshell> new URI("http://foo_bar:8080/").getHost()
   $1 ==> null

   jshell> new URI("http://foo_bar:8080/").getPort()
   $2 ==> -1

   jshell> new URI("http://foobar:8080/").getHost()
   $3 ==> "foobar"

   jshell> new URI("http://foobar:8080/").getPort()
   $4 ==> 8080

2. all tests under "java/net/URI" passed as expected.

3. no failures related my change in tier1, tier2 and tier3 tests.

[dfuch]

I have given this a round of testing and I believe the logic is now right.
I'd suggest adding curly braces around else blocks in case of nested if statements though (see suggestion below) as it makes it easier to verify which belongs to which.

[dfuch]

LGTM

[openjdk]

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

8305763: Parsing a URI with an underscore goes through a silent exception, negatively impacting performance

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

• 9188142: 8309216: Cast from jchar* to char* in test java/io/GetXSpace.java
• d709c25: 8307887: (fs) Files.createSymbolicLink throws less specific exception when in developer mode and file already exists
• ca6f07f: 8309534: @jep(number=430, title="String Templates") should use default status
• 8f0839b: 8308748: JNU_GetStringPlatformChars may write to String's internal memory array
• 01455a0: 8304878: ConcurrentModificationException in javadoc tool
• 7d25bf7: 8309419: RISC-V: Relax register constraint for AddReductionVF & AddReductionVD nodes
• 5146a58: 8309418: RISC-V: Make use of vl1r.v & vfabs.v pseudo-instructions where appropriate
• 41bf2ad: 8308875: java/awt/Toolkit/GetScreenInsetsCustomGC/GetScreenInsetsCustomGC.java failed with 'Cannot invoke "sun.awt.X11GraphicsDevice.getInsets()" because "device" is null'
• a7a0913: 8309346: Extend hs_err logging for all VM operations deriving from VM_GC_Operation
• 3b85f84: 8309340: Provide sctpHandleSocketErrorWithMessage
• ... and 823 more: https://git.openjdk.org/jdk/compare/4485737e9b93cdf5cfc9a0f4dc0f327cd119f75b...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).

[dfuch]

@dhanalla if you integrate this change I will sponsor it. Alternatively if you'd prefer to wait until after the fork that's fine by me.

[dhanalla]

/integrate

[openjdk]

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

[dhanalla]

@dfuch, Thank you for review and sponsorship.
I submitted for integration.

[dfuch]

/sponsor

[openjdk]

Going to push as commit 749d480.
Since your change was applied there have been 852 commits pushed to the master branch:

• 3ccb3c0: 8305906: HttpClient may use incorrect key when finding pooled HTTP/2 connection for IPv6 address
• a25b7b8: 8295976: GetThreadListStackTraces returns wrong state for blocked VirtualThread
• fadcd65: 8309527: Improve test proxy performance
• 0ed4af7: 8309472: IGV: Add dump_igv(custom_name) for improved debugging
• f1c7afc: 8306647: Implementation of Structured Concurrency (Preview)
• a08c5cb: 8307953: [AIX] C locale's font setting was changed by JEP 400
• 0ceb432: 8309570: ProblemList sun/security/pkcs11/Signature/TestRSAKeyLength.java
• 65bdbc7: 8309396: com/sun/jdi/JdbMethodExitTest.java fails with virtual threads due to a bug in determining the main thread id
• 4a75fd4: 8301553: Support Password-Based Cryptography in SunPKCS11
• 0a4f9ad: 8292157: Incorrect error: "block element not allowed within inline element "
• ... and 842 more: https://git.openjdk.org/jdk/compare/4485737e9b93cdf5cfc9a0f4dc0f327cd119f75b...master

Your commit was automatically rebased without conflicts.

[openjdk]

@dfuch @dhanalla Pushed as commit 749d480.

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

[dhanalla]

/backport jdk17u-dev

[openjdk]

@dhanalla To use the /backport command, you need to be in the OpenJDK census and your GitHub account needs to be linked with your OpenJDK username (how to associate your GitHub account with your OpenJDK username).

[dhanalla]

/backport jdk17u-dev

[openjdk]

@dhanalla the backport was successfully created on the branch dhanalla-backport-749d4801 in my personal fork of openjdk/jdk17u-dev. To create a pull request with this backport targeting openjdk/jdk17u-dev:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit 749d4801 from the openjdk/jdk repository.

The commit being backported was authored by Dhamoder Nalla on 7 Jun 2023 and was reviewed by Daniel Fuchs.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk17u-dev:

$ git fetch https://github.com/openjdk-bots/jdk17u-dev.git dhanalla-backport-749d4801:dhanalla-backport-749d4801
$ git checkout dhanalla-backport-749d4801
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk17u-dev.git dhanalla-backport-749d4801