RFR: 8293562: blocked threads with KeepAliveCache.get

[Daniel Fuchs]

On Fri, 23 Sep 2022 06:55:11 GMT, Daniel Jeliński <djelinski at openjdk.org> wrote:

> Please review this patch that makes sure KeepAliveCache does not block all threads while closing sockets.
> 
> Changes:
> - get operation no longer closes sockets; if there's no socket that is recent enough, get returns null and lets the cleaner thread close the sockets
> - put operation closes sockets without holding the cache lock. Additionally, if the cache is full, it places the new connection in the cache and removes the oldest connection.
> - the cleaner thread creates a list of connections to close, and then closes them after releasing the cache lock
> - additionally, we set the socket timeout to 1 millisecond before calling socket.close
> 
> The new test fails with `Wait for second request timed out` without this patch, passes after the changes. Tiers 1-3 clean.

Very good work overall - but I believe we should set the SO_TIMEOUT only for SSLSockets (in the HttpsClient subclass) - and make sure we do it everywhere where serverSocket.close() is called.

src/java.base/share/classes/sun/net/www/http/HttpClient.java line 1163:

> 1161:             keepingAlive = false;
> 1162:             // SSLSocket.close may block up to timeout. Make sure it's short.
> 1163:             serverSocket.setSoTimeout(1);

maybe we should try catch and ignore exception on this line - or close the socket on a finally block. An alternative could be to only do this for SSLSocket - in the HttpsClient - by overriding closeServer() there. Also I see that there are other places where serverSocket.close() is called in the HttpsClient.

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

> 346:             if (size() >= KeepAliveCache.getMaxConnections()) {
> 347:                 // remove oldest connection
> 348:                 staleClient = removeLast().hc;

OK - KeepAliveCache.getMaxConnections() is the maximum number of connections kept alive *per destination*. It's a misnomer but it was there previously.

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

Changes requested by dfuchs (Reviewer).

PR: https://git.openjdk.org/jdk/pull/10401