On Fri, 19 Feb 2021 05:15:24 GMT, Vyom Mani Tewari
<github.com+4410404+vyomm...@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
