On Tue, 7 Feb 2023 15:40:48 GMT, Darragh Clarke <[email protected]> wrote:
>> Currently there is a race condition that can allow for too many
>> 'idleConnections' in `ServerImpl`
>>
>> This PR adds a lock to make sure only one connection can be marked Idle at a
>> time as well as a test that consistently failed before the change but which
>> now passes.
>
> Darragh Clarke has updated the pull request incrementally with one additional
> commit since the last revision:
>
> addressed comments
src/jdk.httpserver/share/classes/sun/net/httpserver/ServerImpl.java line 968:
> 966: boolean close = false;
> 967:
> 968: idleConnectionLock.lock();
I am wondering if this new lock is needed to deal with this change. The
`idleConnections` is already a `Collections.synchronizedSet`, so maybe we could
just use it here as follows:
boolean close = false;
synchronized(idleConnections) {
if (idleConnections.size() >= MAX_IDLE_CONNECTIONS) {
// closing the connection here could block
// instead set boolean and close outside of synchronized block
close = true;
} else {
c.idleStartTime = System.currentTimeMillis();
c.setState(State.IDLE);
idleConnections.add(c);
}
}
if (close) {
c.close();
allConnections.remove(c);
}
The reason I propose using `synchronization` on the existing collection is
because, that's the "lock" we use in various other places while dealing with
the `idleConnections` set. So using `synchronized` here would provide
consistency when handling the `idleConnections` set. Furthermore, with the
introduction of this new `idleConnectionLock`, this method is the sole place we
use it and other parts of the code (like the idle timeout task) would then use
a different "monitor" while dealing with the `idleConnections` set and could
potentially lead to some odd issues, I think.
-------------
PR: https://git.openjdk.org/jdk/pull/12413
