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

[Daniel Fuchs]

On Wed, 8 Feb 2023 11:45:47 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> 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.

Good point, and good catch!

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

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