RFR: 8300268 : ServerImpl allows too many idle connections when using sun.net.httpserver.maxIdleConnections [v3]

[Jaikiran Pai]

On Tue, 7 Feb 2023 15:40:48 GMT, Darragh Clarke <duke at openjdk.org> 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