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.

Sign up for GitHub

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

8302732: sun/net/www/http/HttpClient/MultiThreadTest.java still failing intermittently #12676

Closed
wants to merge 5 commits into from
+54 −73
Closed

8302732: sun/net/www/http/HttpClient/MultiThreadTest.java still failing intermittently #12676

Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3883285
Fix possible race
djelinski Feb 20, 2023
f6edc7b
Delay closing the sweeper thread
djelinski Feb 21, 2023
de6f329
Reword
djelinski Feb 21, 2023
bb5596c
Remove unnecessary lock
djelinski Feb 23, 2023
0337c28
Add asserts
djelinski Feb 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 40 additions & 60 deletions src/java.base/share/classes/sun/net/www/http/KeepAliveCache.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1996, 2022, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1996, 2023, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -36,7 +36,6 @@
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;

import jdk.internal.misc.InnocuousThread;
@@ -243,32 +242,32 @@ public void run() {
// Remove all outdated HttpClients.
cacheLock.lock();
try {
if (isEmpty()) {
// cache not used in the last LIFETIME - exit
keepAliveTimer = null;
break;
}
long currentTime = System.currentTimeMillis();
List<KeepAliveKey> keysToRemove = new ArrayList<>();

for (KeepAliveKey key : keySet()) {
ClientVector v = get(key);
v.lock();
try {
KeepAliveEntry e = v.peekLast();
while (e != null) {
if ((currentTime - e.idleStartTime) > v.nap) {
v.pollLast();
if (closeList == null) {
closeList = new ArrayList<>();
}
closeList.add(e.hc);
} else {
break;
KeepAliveEntry e = v.peekLast();
while (e != null) {
if ((currentTime - e.idleStartTime) > v.nap) {
v.pollLast();
if (closeList == null) {
closeList = new ArrayList<>();
}
e = v.peekLast();
closeList.add(e.hc);
} else {
break;
}
e = v.peekLast();
}

if (v.isEmpty()) {
keysToRemove.add(key);
}
} finally {
v.unlock();
if (v.isEmpty()) {
keysToRemove.add(key);
}
}

@@ -284,7 +283,7 @@ public void run() {
}
}
}
} while (!isEmpty());
} while (keepAliveTimer == Thread.currentThread());
}

/*
@@ -309,7 +308,6 @@ private void readObject(ObjectInputStream stream)
class ClientVector extends ArrayDeque<KeepAliveEntry> {
@java.io.Serial
private static final long serialVersionUID = -8680532108106489459L;
private final ReentrantLock lock = new ReentrantLock();

// sleep time in milliseconds, before cache clear
int nap;
@@ -320,55 +318,37 @@ class ClientVector extends ArrayDeque<KeepAliveEntry> {

/* return a still valid, idle HttpClient */
HttpClient get() {
lock();
try {
// check the most recent connection, use if still valid
KeepAliveEntry e = peekFirst();
if (e == null) {
return null;
}
long currentTime = System.currentTimeMillis();
if ((currentTime - e.idleStartTime) > nap) {
return null; // all connections stale - will be cleaned up later
} else {
pollFirst();
if (KeepAliveCache.logger.isLoggable(PlatformLogger.Level.FINEST)) {
String msg = "cached HttpClient was idle for "
+ Long.toString(currentTime - e.idleStartTime);
KeepAliveCache.logger.finest(msg);
}
return e.hc;
// check the most recent connection, use if still valid
Copy link
Member

Choose a reason for hiding this comment

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

It might be prudent to assert that the global cacheLock is held by the current thread in get & put (if that's feasible without too much hackery). Just worrying about future maintainers possibly modifying this code and using ClientVector outside of the lock. The assert would make sure they ponder the consequences.

Copy link
Member Author

Choose a reason for hiding this comment

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

Asserts added; let me know if the amount of hackery involved looks acceptable.

Copy link
Member

Choose a reason for hiding this comment

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

Looks reasonable. I personally dislike having several top-level classes in the same file so I'd say having ClientVector as an inner class is a plus. Having ClientVector carry a (hidden) pointer to its parent instance shouldn't be an issue since ClientVector shouldn't escape. I'd say this is good, provided that the modified test still passes. Otherwise, we can replace the assert with a simple comment, to warn that the class is not thread-safe and should not be used outside of blocks protected by the cacheLock.

KeepAliveEntry e = peekFirst();
if (e == null) {
return null;
}
long currentTime = System.currentTimeMillis();
if ((currentTime - e.idleStartTime) > nap) {
return null; // all connections stale - will be cleaned up later
} else {
pollFirst();
if (KeepAliveCache.logger.isLoggable(PlatformLogger.Level.FINEST)) {
String msg = "cached HttpClient was idle for "
+ Long.toString(currentTime - e.idleStartTime);
KeepAliveCache.logger.finest(msg);
}
} finally {
unlock();
return e.hc;
}
}

HttpClient put(HttpClient h) {
HttpClient staleClient = null;
lock();
try {
assert KeepAliveCache.getMaxConnections() > 0;
if (size() >= KeepAliveCache.getMaxConnections()) {
// remove oldest connection
staleClient = removeLast().hc;
}
addFirst(new KeepAliveEntry(h, System.currentTimeMillis()));
} finally {
unlock();
assert KeepAliveCache.getMaxConnections() > 0;
if (size() >= KeepAliveCache.getMaxConnections()) {
// remove oldest connection
staleClient = removeLast().hc;
}
addFirst(new KeepAliveEntry(h, System.currentTimeMillis()));
// close after releasing the locks
return staleClient;
}

final void lock() {
lock.lock();
}

final void unlock() {
lock.unlock();
}

/*
* Do not serialize this class!
*/