On Wed, 8 Feb 2023 11:45:47 GMT, Jaikiran Pai <[email protected]> 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
