8297976: Remove sun.net.ProgressMonitor and related classes #11474

djelinski · 2022-12-02T08:31:25Z

Please review this patch that removes progress monitoring classes used by UrlConnection.
Since Java 9 these classes are not used in the JDK, and are not exported from java.base. If anyone was still using them, reimplementing them in user code should be pretty straightforward.

This PR also fixes the issue where MeteredStream finalizer could resurrect an unusable connection, causing unexpected exceptions in other parts of the code.

No new regression test; since we are removing the finalizer, I don't believe we will see the issue again. I can add a test for that if you think it still makes sense.

I had to adjust ProxyModuleMapping.java test which used sun.net.ProgressListener as an example of a module-private interface; I replaced it with another public interface from the same package.

Existing tier 1-3 tests continue to pass.

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

Issues

JDK-8297976: Remove sun.net.ProgressMonitor and related classes
JDK-8240275: Occasional errors in HttpURLConnection due to race with GC

Reviewers

Jaikiran Pai (@jaikiran - Reviewer)
Daniel Fuchs (@dfuch - Reviewer)
Michael McMahon (@Michael-Mc-Mahon - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11474

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

Using diff file

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

djelinski · 2022-12-02T08:31:47Z

/issue add 8240275

bridgekeeper · 2022-12-02T08:33:19Z

👋 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 · 2022-12-02T08:36:22Z

@djelinski
Adding additional issue to issue list: 8240275: Occasional errors in HttpURLConnection due to race with GC.

openjdk · 2022-12-02T08:40:04Z

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

core-libs
net

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

mlbridge · 2022-12-02T08:57:22Z

Webrevs

dfuch · 2022-12-02T10:25:35Z

JDK-8240275 seems to have a reproducer attached. Did you investigate whether turning that into a non-regression test would be worthwhile?

Otherwise this looks like a good cleanup and simplification!

src/java.base/share/classes/sun/net/www/http/KeepAliveStream.java

jaikiran

Looks good to me (apart from Daniel's question).

As you note, these are internal classes and aren't exported from the java.base module (to applications) and given the context in which those classes were used, my understanding is that we won't need a CSR for this change.

openjdk · 2022-12-05T09:21:55Z

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

8297976: Remove sun.net.ProgressMonitor and related classes
8240275: Occasional errors in HttpURLConnection due to race with GC

Reviewed-by: jpai, dfuchs, michaelm

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

8ea369a: 8298171: Missing newline in the example output of -Xlog:help
acf96c6: 8290432: C2 compilation fails with assert(node->_last_del == _last) failed: must have deleted the edge just produced
ce89673: 8297804: (tz) Update Timezone Data to 2022g
62baff5: 8298221: Problem list gc/metaspace/CompressedClassSpaceSizeInJmapHeap.java on macosx-aarch64
16a5901: 8298214: ProblemList java/util/concurrent/forkjoin/AsyncShutdownNow.java
b4da0ee: 8296507: GCM using more memory than necessary with in-place operations
cd2182a: 8295724: VirtualMachineError: Out of space in CodeCache for method handle intrinsic
2cdc019: 8298178: Update to use jtreg 7.1.1
79d163d: 8293412: Remove unnecessary java.security.egd overrides
ea83cb9: 8297450: ScaledTextFieldBorderTest.java fails when run with -show parameter
... and 94 more: https://git.openjdk.org/jdk/compare/cd776093c79e9a1a4c40c0adfdbfeedf293d99c7...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.

djelinski · 2022-12-05T14:06:23Z

The reproducer from JDK-8240275 didn't reliably reproduce the issue because some of the finalizers were replaced by cleaners in the meantime.
I added a test that verifies that the socket used by HTTPSUrlConnection is not reused after its finalizer completes. The test passes with this PR, fails with the current master.

Co-authored-by: Andrey Turbanov <turbanoff@gmail.com>

test/jdk/sun/net/www/http/KeepAliveStream/KeepAliveStreamFinalizer.java

jaikiran · 2022-12-06T12:07:35Z

test/jdk/sun/net/www/http/KeepAliveStream/KeepAliveStreamFinalizer.java

+ * @test
+ * @bug 8240275
+ * @library /test/lib
+ * @run main/othervm KeepAliveStreamFinalizer


Hello Daniel, it doesn't look like we are doing anything JVM specific in this test. Is the othervm intentional here?

The test calls HttpsURLConnection.setDefaultSSLSocketFactor which requires using /othervm

Ah yes, overlooked that part.

dfuch

LGTM

Michael-Mc-Mahon

Looks fine to me.

jaikiran

Looks good to me.

djelinski · 2022-12-07T08:54:44Z

Thanks for the reviews!

/integrate

openjdk · 2022-12-07T08:56:30Z

Going to push as commit 27bbe7b.
Since your change was applied there have been 106 commits pushed to the master branch:

085f96c: 8295258: Add BasicType argument to AccessInternal::decorator_fixup
1c2a093: 8298147: Clang warns about pointless comparisons
8ea369a: 8298171: Missing newline in the example output of -Xlog:help
acf96c6: 8290432: C2 compilation fails with assert(node->_last_del == _last) failed: must have deleted the edge just produced
ce89673: 8297804: (tz) Update Timezone Data to 2022g
62baff5: 8298221: Problem list gc/metaspace/CompressedClassSpaceSizeInJmapHeap.java on macosx-aarch64
16a5901: 8298214: ProblemList java/util/concurrent/forkjoin/AsyncShutdownNow.java
b4da0ee: 8296507: GCM using more memory than necessary with in-place operations
cd2182a: 8295724: VirtualMachineError: Out of space in CodeCache for method handle intrinsic
2cdc019: 8298178: Update to use jtreg 7.1.1
... and 96 more: https://git.openjdk.org/jdk/compare/cd776093c79e9a1a4c40c0adfdbfeedf293d99c7...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-12-07T08:56:44Z

@djelinski Pushed as commit 27bbe7b.

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

djelinski added 4 commits December 1, 2022 17:53

Remove progress monitoring classes

d412599

Remove broken MeteredStream finalizer

f6b7421

Update broken test

ddd02eb

Update copyright

e39aae1

openjdk bot changed the title ~~8297976~~ 8297976: Remove sun.net.ProgressMonitor and related classes Dec 2, 2022

openjdk bot added core-libs core-libs-dev@openjdk.org net net-dev@openjdk.org labels Dec 2, 2022

djelinski marked this pull request as ready for review December 2, 2022 08:50

openjdk bot added the rfr Pull request is ready for review label Dec 2, 2022

turbanoff reviewed Dec 5, 2022

View reviewed changes

src/java.base/share/classes/sun/net/www/http/KeepAliveStream.java Outdated Show resolved Hide resolved

jaikiran approved these changes Dec 5, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 5, 2022

Add test

e0e157c

Remove double space

8d7c6e5

Co-authored-by: Andrey Turbanov <turbanoff@gmail.com>

jaikiran reviewed Dec 6, 2022

View reviewed changes

test/jdk/sun/net/www/http/KeepAliveStream/KeepAliveStreamFinalizer.java Outdated Show resolved Hide resolved

jaikiran reviewed Dec 6, 2022

View reviewed changes

dfuch approved these changes Dec 6, 2022

View reviewed changes

Use System.err

9be8eea

Michael-Mc-Mahon approved these changes Dec 6, 2022

View reviewed changes

dfuch approved these changes Dec 6, 2022

View reviewed changes

jaikiran approved these changes Dec 7, 2022

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Dec 7, 2022

openjdk bot closed this Dec 7, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 7, 2022

djelinski deleted the meteredstream branch December 7, 2022 11:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8297976: Remove sun.net.ProgressMonitor and related classes #11474

8297976: Remove sun.net.ProgressMonitor and related classes #11474

djelinski commented Dec 2, 2022 •

edited by openjdk bot

djelinski commented Dec 2, 2022

bridgekeeper bot commented Dec 2, 2022

openjdk bot commented Dec 2, 2022

openjdk bot commented Dec 2, 2022

mlbridge bot commented Dec 2, 2022 •

edited

dfuch commented Dec 2, 2022

jaikiran left a comment •

edited

openjdk bot commented Dec 5, 2022 •

edited

djelinski commented Dec 5, 2022

jaikiran Dec 6, 2022

dfuch Dec 6, 2022

jaikiran Dec 6, 2022

dfuch left a comment

Michael-Mc-Mahon left a comment

jaikiran left a comment

djelinski commented Dec 7, 2022

openjdk bot commented Dec 7, 2022

openjdk bot commented Dec 7, 2022

8297976: Remove sun.net.ProgressMonitor and related classes #11474

8297976: Remove sun.net.ProgressMonitor and related classes #11474

Conversation

djelinski commented Dec 2, 2022 • edited by openjdk bot

Progress

Issues

Reviewers

Reviewing

djelinski commented Dec 2, 2022

bridgekeeper bot commented Dec 2, 2022

openjdk bot commented Dec 2, 2022

openjdk bot commented Dec 2, 2022

mlbridge bot commented Dec 2, 2022 • edited

Webrevs

dfuch commented Dec 2, 2022

jaikiran left a comment • edited

Choose a reason for hiding this comment

openjdk bot commented Dec 5, 2022 • edited

djelinski commented Dec 5, 2022

jaikiran Dec 6, 2022

Choose a reason for hiding this comment

dfuch Dec 6, 2022

Choose a reason for hiding this comment

jaikiran Dec 6, 2022

Choose a reason for hiding this comment

dfuch left a comment

Choose a reason for hiding this comment

Michael-Mc-Mahon left a comment

Choose a reason for hiding this comment

jaikiran left a comment

Choose a reason for hiding this comment

djelinski commented Dec 7, 2022

openjdk bot commented Dec 7, 2022

openjdk bot commented Dec 7, 2022

djelinski commented Dec 2, 2022 •

edited by openjdk bot

mlbridge bot commented Dec 2, 2022 •

edited

jaikiran left a comment •

edited

openjdk bot commented Dec 5, 2022 •

edited