[jdk11u-dev] RFR: 8293562: KeepAliveCache Blocks Threads while Closing Connections

[Severin Gehwolf]

On Fri, 7 Apr 2023 13:00:28 GMT, Henry <duke at openjdk.org> wrote:

> 8299602: blocked threads with KeepAliveCache.
> 
> I followed the steps that helped me carry out this pull-request.
> 
> REF/: https://mail.openjdk.org/pipermail/jdk-updates-dev/2023-February/020439.html
> 
> Thanks to @GoeLin and @jerboaa

This patch is pretty hard to review since it includes many unrelated changes. Please try to only make *necessary* adaptations and explain why those changes needed to be made.

Perhaps it makes more sense to start over by applying the JDK 17u patch and adjusting it until it works and you're confident the patch does what it's supposed to do. It's important to configure your editor in a way to not modify code that isn't being touched by the patch. Unrelated formatting changes are not OK in a backport.

While the JDK 17u patch builds on https://bugs.openjdk.org/browse/JDK-8229867, this backport should not include those changes. It should use the old synchronization scheme instead.

src/java.base/share/classes/sun/net/www/http/KeepAliveCache.java line 75:

> 73:     static int getUserKeepAliveSeconds(String type) {
> 74:         int v = AccessController.doPrivileged(
> 75:             new GetIntegerAction(keepAliveProp + type, -1)).intValue();

Please refrain from changing style in a backport.

src/java.base/share/classes/sun/net/www/http/KeepAliveCache.java line 95:

> 93:     static int result = -1;
> 94: 
> 95:     @SuppressWarnings("removal")

This is not part of the original patch (at least not in the JDK 17u version of the fix). See https://git.openjdk.org/jdk17u/commit/244d1942d2e9f2ba75267acc16a02041c1e8080e

src/java.base/share/classes/sun/net/www/http/KeepAliveCache.java line 111:

> 109: 
> 110:     // This class is never serialized (see writeObject/readObject).
> 111:     private final ReentrantLock cacheLock = new ReentrantLock();

This got introduced in JDK 16 with https://bugs.openjdk.org/browse/JDK-8229867, please don't introduce such changes in a backport. The patch needs to be done without JDK-8229867 being present in JDK 11u

src/java.base/share/classes/sun/net/www/http/KeepAliveCache.java line 119:

> 117:      */
> 118:     public KeepAliveCache() {
> 119:     }

Unrelated style change.

src/java.base/share/classes/sun/net/www/http/KeepAliveCache.java line 127:

> 125:      * @param http The HttpClient to be cached
> 126:      */
> 127:     @SuppressWarnings("removal")

Those annotations aren't part of the original change.

src/java.base/share/classes/sun/net/www/http/KeepAliveCache.java line 212:

> 210: 
> 211:     /*
> 212:      * called by a clientVector thread when all its connections have timed out

Unrelated style change

src/java.base/share/classes/sun/net/www/http/KeepAliveCache.java line 216:

> 214:      */
> 215:     private void removeVector(KeepAliveKey k) {
> 216:         assert cacheLock.isHeldByCurrentThread();

Not part of the original changeset. Please keep the port as close as possible to the JDK 17 version. Caveats such as JDK-8229867 apply.

-------------

Changes requested by sgehwolf (Reviewer).

PR Review: https://git.openjdk.org/jdk11u-dev/pull/1825#pullrequestreview-1399931322
PR Review Comment: https://git.openjdk.org/jdk11u-dev/pull/1825#discussion_r1176523700
PR Review Comment: https://git.openjdk.org/jdk11u-dev/pull/1825#discussion_r1176539475
PR Review Comment: https://git.openjdk.org/jdk11u-dev/pull/1825#discussion_r1176537849
PR Review Comment: https://git.openjdk.org/jdk11u-dev/pull/1825#discussion_r1176541345
PR Review Comment: https://git.openjdk.org/jdk11u-dev/pull/1825#discussion_r1176542430
PR Review Comment: https://git.openjdk.org/jdk11u-dev/pull/1825#discussion_r1176543081
PR Review Comment: https://git.openjdk.org/jdk11u-dev/pull/1825#discussion_r1176545700