On Fri, 19 Feb 2021 04:26:26 GMT, Jaikiran Pai <[email protected]> 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.
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.
-------------
PR: https://git.openjdk.java.net/jdk/pull/2601
