On Fri, 23 Sep 2022 06:55:11 GMT, Daniel Jeliński <[email protected]> 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
