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 413:
> 411: }
> 412: responseCompleted (c);
> 413: if (t.close || idleConnections.size() >=
> MAX_IDLE_CONNECTIONS) {
The root cause of the issue appears to be that, before this change, the code
execution, at time T1, would notice that the `idleConnections` set hasn't yet
reached the configured limit and would thus skip the closing of the connection.
However, it wouldn't immediately add this unclosed connection to the
`idleConnections` Set and instead would add it to a temporary collection (the
`connsToRegister`) and at a later time T2 would then iterate over the temporary
`connsToRegister` collection and mark the connections in that collection as
idle. That would thus leave a (considerable) period of time between T1 and T2
where some other threads could reach this same code and notice that the idle
connection set hasn't yet reached the limit, but they wouldn't consider the
connections that have been temporarily added to the `connsToRegister` set, in
this limit computation. Thus it would lead to a situation where, when the
connections are ultimately added to the `idleConnections` set, in `markIdle(
) method`, they would potentially exceed the limit.
The change in this PR proposes that `markIdle()` method take the responsibility
of checking the idle connection limit and deciding whether or not to accomodate
the connection in that Set. I think, that's a good thing.
-------------
PR: https://git.openjdk.org/jdk/pull/12413
