RFR: 8260366: ExtendedSocketOptions <clinit> can deadlock in some circumstances [v4]

[Jaikiran Pai]

On Fri, 19 Feb 2021 05:15:24 GMT, Vyom Mani Tewari <github.com+4410404+vyommani at openjdk.org> wrote:

>> After thinking a bit more about this issue and the patch I had proposed, I realized what I did wrong in this patch. The `synchronized` block in the `sun.net.ext.ExtendedSocketOptions#getInstance()` was scoped one statement too many. We don't (and shouldn't) need the `Class.forName("jdk.net.ExtendedSocketOptions")` to be part of that synchronized block. I've now modified and fixed that part to have the synchronized block only at the place where it should be needed. I've also enhanced the test to introduce a couple of additional concurrent tasks which now call `sun.net.ext.ExtendedSocketOptions#getInstance()`. So in total, the test now fires off 4 tasks, 2 of which load the respective classes and the other 2 call the getInstance() method, all concurrently. This should make the test a bit more robust and should catch any potential deadlocks. With this fix, the test now passes.
>> 
>> I've now updated and reopened this PR for further reviews.
>
>> After thinking a bit more about this issue and the patch I had proposed, I realized what I did wrong in this patch. The `synchronized` block in the `sun.net.ext.ExtendedSocketOptions#getInstance()` was scoped one statement too many. We don't (and shouldn't) need the `Class.forName("jdk.net.ExtendedSocketOptions")` to be part of that synchronized block. I've now modified and fixed that part to have the synchronized block only at the place where it should be needed. I've also enhanced the test to introduce a couple of additional concurrent tasks which now call `sun.net.ext.ExtendedSocketOptions#getInstance()`. So in total, the test now fires off 4 tasks, 2 of which load the respective classes and the other 2 call the getInstance() method, all concurrently. This should make the test a bit more robust and should catch any potential deadlocks. With this fix, the test now passes.
>> 
>> I've now updated and reopened this PR for further reviews.
> 
> Hi Jaikiran,
> 
> I looked into your previous patch and i believe why you were observing the deadlock because you make "sun.net.ext.ExtendedSocketOption.register" thread safe(synchronize). When you are loading both the classes simultaneously one thread is calling getInstance()  and other thread while loading the class(jdk.net.ExtendedSocketOptions) calls  register  and you are observing the deadlock.
> 
> Although sun.net.ext.ExtendedSocketOptions.register is public method but it will be called only from jdk.net.ExtendedSocketOption static block, so we don't need it to be thread safe.
> 
> Let's wait for what others people say.

Any reviews/comments, in addition to what Vyom mentioned, to the updated PR please?

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

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