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

[Daniel Fuchs]

On Fri, 19 Feb 2021 04:29:55 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Can I please get a review for this change which proposes to fix the issue reported in https://bugs.openjdk.java.net/browse/JDK-8260366?
>> 
>> The issue relates to the concurrent classloading of `sun.net.ext.ExtendedSocketOptions` and `jdk.net.ExtendedSocketOptions` leading to a deadlock. This is because the `sun.net.ext.ExtendedSocketOptions` in its static block does a `Class.forName("jdk.net.ExtendedSocketOptions")`. The `jdk.net.ExtendedSocketOptions` in its own static block calls the `register` method on `sun.net.ext.ExtendedSocketOptions`. If 2 threads concurrently try loading these classes (one loading the `sun.net.ext.ExtendedSocketOptions` and the other loading `jdk.net.ExtendedSocketOptions`), it can so happen that each one ends up holding a classloading lock in the static block of the respective class, while waiting for the other thread to release the lock, thus leading to a deadlock. The stacktrace posted in the linked JBS issue has the necessary details on the deadlock.
>> 
>> The commit here breaks this deadlock by moving out the `Class.forName("jdk.net.ExtendedSocketOptions")` call from the static block of `sun.net.ext.ExtendedSocketOptions` to the `getInstance()` method, thus lazily loading (on first call to `getInstance()`) and registering the `jdk.net.ExtendedSocketOptions`.
>> 
>> Extra attention needs to be given to the `sun.net.ext.ExtendedSocketOptions#register(ExtendedSocketOptions extOptions)` method. Before the change in this PR, when the `sun.net.ext.ExtendedSocketOptions` would successfully complete loading, it was guaranteed that the registered `ExtendedSocketOptions` would either be the one registered from the `jdk.net.ExtendedSocketOptions` or a `NoExtendedSocketOptions`. There wasn't any window of chance for any code (be it in the JDK or in application code) to call the `sun.net.ext.ExtendedSocketOptions#register` to register any different/other implementation/instance of the `ExtendedSocketOptions`. However, with this change in the PR, there is now a window of chance where any code in the JDK (or even application code?) can potentially call the `sun.net.ext.ExtendedSocketOptions#register` before either the `jdk.net.ExtendedSocketOptions` is loaded or the `sun.net.ext.ExtendedSocketOptions#getInstance()` method is called, thus allowing for some 
 other implementation of the `ExtendedSocketOptions` to be registered. However, I'm not sure if it's a practical scenario - although the `sun.net.ext.ExtendedSocketOptions#register` is marked `public`, the comment on that method and the fact that it resides in an internal, not exposed by default class/module, makes me believe that this `register` method isn't supposed to be called by anyone other than the `jdk.net.ExtendedSocketOptions`. If at all this `register` method is allowed to be called from other places, then due to the change in this PR, additional work needs to be probably done in its implementation to allow for the `jdk.net.ExtendedSocketOptions` to be given first priority(?) to be registered first. I'll need input on whether I should worry about this case or if it's fine in its current form in this PR.
>> 
>> This PR also contains a jtreg test which reproduces the issue and verifies the fix.
>
> Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Reduce the scope of the synchronized block in getInstance() and enhance the testcase to be more robust in catching the deadlocks

The fix and test looks good to me.

src/java.base/share/classes/sun/net/ext/ExtendedSocketOptions.java line 199:

> 197: 
> 198:         instance = extOptions;
> 199:     }

Arguably, because `instance` is volatile, you could also use the double locking mechanism here to throw before synchronizing if instance is already set, and check again after synhronizing. However, it's probably not worth it since this method is expected to be called (and should always be called) only once - so I believe what you have here should be enough.

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

Marked as reviewed by dfuchs (Reviewer).

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