RFR: 8277969: HttpClient SelectorManager shuts down when custom Executor rejects a task [v2]

[Daniel Fuchs]

On Thu, 10 Mar 2022 10:08:16 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> tryRelease can be called multiple time - it will only decrement the ref counting once. It could happen when different threads notice that the operation is finished (usually due to some exceptions being propagated) and try concurrently to decrease the ref counter. It is very important to decrease the ref counter (otherwise the HttpClient may never shutdown when it's released) and equally very important not to decrease it twice for the same operation. Here we don't need to check whether it's been acquired because we know it's been acquired if we reach here. IIRC what we don't want to do is call tryRelease() if it's never been acquired.
>
>> Here we don't need to check whether it's been acquired because we know it's been acquired if we reach here.
> 
> Hello Daniel, may be I am misreading the diff but from what I can see, the `acquire()` method now returns a `boolean` which we are assigning to the local `acquire`. But we aren't checking it for `true` for doing the subsequent operation. So when it reaches the finally block here, as far as I can see, it's not guaranteed that we did indeed `acquire` it.

The acquire() method will return true the first time it's been called. And it is called only once. So we only need to check whether `acquired` is true at places where we are in doubt about whether the method has been called.
The code that calls acquire() above could simply have ignored the result of `acquire()` and set the boolean to `true`. That said, it would not be wrong to check whether `acquired==true` here too - maybe I should do it for consistency...

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

PR: https://git.openjdk.java.net/jdk/pull/7196