8301964: Expensive fillInStackTrace operation in HttpURLConnection.getLastModified when no last-modified in response #12451

turbanoff · 2023-02-07T09:19:06Z

JDK-8297211 (#11258) missed one case with date headers: last-modified, date, expires. We can avoid expensive NPE.

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-8301964: Expensive fillInStackTrace operation in HttpURLConnection.getLastModified when no last-modified in response

Reviewers

Daniel Fuchs (@dfuch - Reviewer) ⚠️ Review applies to b836a2c3
Jaikiran Pai (@jaikiran - Reviewer) ⚠️ Review applies to b836a2c3
Alan Bateman (@AlanBateman - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12451

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

Using diff file

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

…LastModified when no last-modified in response

bridgekeeper · 2023-02-07T09:20:28Z

👋 Welcome back aturbanov! 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 · 2023-02-07T09:22:52Z

@turbanoff 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 · 2023-02-07T10:59:39Z

Webrevs

AlanBateman · 2023-02-07T11:05:04Z

src/java.base/share/classes/java/net/HttpURLConnection.java

+                dateString = dateString + " GMT";
+            }
+            try {
+                return Date.parse(dateString);


Changing this method makes me wonder if we should replace the Date.parse usage with DateTimeFormatter while we are there, also the parameter name "Default" wants to be renamed too.

I've renamed Default to defaultValue.
Regarding Date.parse replacement: I think it's a quite non-trivial task, which deserves a separate issue. As I see, there already was issue created and closed as Won't Fix - JDK-4097762

…tLastModified when no last-modified in response rename Default to defaultValue

dfuch

Looks like a good cleanup. Thanks for renaming Default into defaultValue.

openjdk · 2023-02-07T15:02:37Z

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

8301964: Expensive fillInStackTrace operation in HttpURLConnection.getLastModified when no last-modified in response

Reviewed-by: dfuchs, jpai, alanb

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

a917fb3: 7033677: potential cast error in MemberEnter
6120319: 8302226: failure_handler native.core should wait for coredump to finish
fef3eab: 8302734: Parallel: Remove unused LGRPSpace::_invalid_region
ea5bfea: 8302661: Parallel: Remove PSVirtualSpace::is_aligned
edf238b: 8302635: Race condition in HttpBodySubscriberWrapper when cancelling request
cd77fcf: 8290822: C2: assert in PhaseIdealLoop::do_unroll() is subject to undefined behavior
57c9bc3: 8302335: IGV: Bytecode not showing
57fde75: 8302113: Improve CRC32 intrinsic with crypto pmull on AArch64
b8c9d6c: 8302158: PPC: test/jdk/jdk/internal/vm/Continuation/Fuzz.java: AssertionError: res: false shouldPin: false
dc55a7f: 8302202: Incorrect desugaring of null-allowed nested patterns
... and 156 more: https://git.openjdk.org/jdk/compare/9dad874ff9f03f5891aa8b37e7826a67c851f06d...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.

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

jaikiran

Hello Andrey, the changes look fine to me. Please update the copyright years on URLConnection.java and HttpsURLConnectionImpl.java files before integrating.

…tLastModified when no last-modified in response update copyright year

AlanBateman

The changes looks okay, I just wonder why the try-catch around Date.parse catches Exception, should it be IllegalArgumentException?

turbanoff · 2023-02-12T09:10:52Z

Javadoc of the Date.parse method does not mention which exceptions could be thrown. That's why I decided to be on the safe side and do not change exception type.

AlanBateman · 2023-02-12T09:25:01Z

Javadoc of the Date.parse method does not mention which exceptions could be thrown. That's why I decided to be on the safe side and do not change exception type.

That's a bug in its javadoc. It may be deprecated but it should specify that it throws IAE if the input cannot be parsed.

dfuch · 2023-02-13T12:37:27Z

Javadoc of the Date.parse method does not mention which exceptions could be thrown. That's why I decided to be on the safe side and do not change exception type.

That's a bug in its javadoc. It may be deprecated but it should specify that it throws IAE if the input cannot be parsed.

Yes - thank you. This is supposed to be a simple cleanup so I'm feeling more comfortable that the logic hasn't changed. The code before the change was also catching Exception.

AlanBateman · 2023-02-13T13:26:29Z

Yes - thank you. This is supposed to be a simple cleanup so I'm feeling more comfortable that the logic hasn't changed. The code before the change was also catching Exception.

We can create a bug to replace the catch Exception with catch IllegalArgumentException, better still be to replace this usage (surprised it wasn't replaced a long time ago).

turbanoff · 2023-02-25T13:37:27Z

/integrate

openjdk · 2023-02-25T13:39:41Z

Going to push as commit 3a9f491.
Since your change was applied there have been 277 commits pushed to the master branch:

1dbd18a: 8302120: Prefer ArrayList to LinkedList in AggregatePainter
2e78d83: 8302899: Executors.newSingleThreadExecutor can use Cleaner to shutdown executor
17e3769: 8302871: Speed up StringLatin1.regionMatchesCI
b4ea807: 8288912: vmTestbase/nsk/stress/strace/strace002.java fails with Unexpected method name: currentCarrierThread
ccf3340: 8303083: (bf) Remove private DirectByteBuffer(long, int) constructor before JDK 21 GA
83d77b1: 8303072: Memory leak in exeNullCallerTest.cpp
7d8b8ba: 8303131: pandoc.exe mangles all processed html files
8f7c496: 8302810: NMT gtests don't correctly check for marked ranges
1a07871: 8302173: Button border overlaps with button icon on macOS system LaF
796cdd5: 8302677: JFR: Cache label and contentType in EventType and ValueDescriptor
... and 267 more: https://git.openjdk.org/jdk/compare/9dad874ff9f03f5891aa8b37e7826a67c851f06d...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-02-25T13:39:53Z

@turbanoff Pushed as commit 3a9f491.

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

[PATCH] Expensive fillInStackTrace operation in HttpURLConnection.get…

a585c3c

…LastModified when no last-modified in response

openjdk bot added the net net-dev@openjdk.org label Feb 7, 2023

turbanoff changed the title ~~[PATCH] Expensive fillInStackTrace operation in HttpURLConnection.getLastModified when no last-modified in response~~ 8301964: Expensive fillInStackTrace operation in HttpURLConnection.getLastModified when no last-modified in response Feb 7, 2023

openjdk bot added the rfr Pull request is ready for review label Feb 7, 2023

AlanBateman reviewed Feb 7, 2023

View reviewed changes

8301964: Expensive fillInStackTrace operation in HttpURLConnection.ge…

b836a2c

…tLastModified when no last-modified in response rename Default to defaultValue

dfuch approved these changes Feb 7, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 7, 2023

jaikiran approved these changes Feb 8, 2023

View reviewed changes

8301964: Expensive fillInStackTrace operation in HttpURLConnection.ge…

e2690ee

…tLastModified when no last-modified in response update copyright year

turbanoff requested a review from AlanBateman February 8, 2023 16:00

AlanBateman approved these changes Feb 12, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Feb 25, 2023

openjdk bot closed this Feb 25, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 25, 2023

8301964: Expensive fillInStackTrace operation in HttpURLConnection.getLastModified when no last-modified in response #12451

8301964: Expensive fillInStackTrace operation in HttpURLConnection.getLastModified when no last-modified in response #12451

Uh oh!

Conversation

turbanoff commented Feb 7, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Feb 7, 2023

Uh oh!

openjdk bot commented Feb 7, 2023

Uh oh!

mlbridge bot commented Feb 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

AlanBateman Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

turbanoff Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

dfuch left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Feb 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaikiran left a comment

Choose a reason for hiding this comment

Uh oh!

AlanBateman left a comment

Choose a reason for hiding this comment

Uh oh!

turbanoff commented Feb 12, 2023

Uh oh!

AlanBateman commented Feb 12, 2023

Uh oh!

dfuch commented Feb 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlanBateman commented Feb 13, 2023

Uh oh!

turbanoff commented Feb 25, 2023

Uh oh!

openjdk bot commented Feb 25, 2023

Uh oh!

openjdk bot commented Feb 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

turbanoff commented Feb 7, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Feb 7, 2023 •

edited

Loading

openjdk bot commented Feb 7, 2023 •

edited

Loading

dfuch commented Feb 13, 2023 •

edited

Loading