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 oth
er 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.
-------------
Commit messages:
- 8260366: ExtendedSocketOptions <clinit> can deadlock in some circumstances
Changes: https://git.openjdk.java.net/jdk/pull/2601/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2601&range=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8260366
Stats: 126 lines in 2 files changed: 112 ins; 11 del; 3 mod
Patch: https://git.openjdk.java.net/jdk/pull/2601.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2601/head:pull/2601
PR: https://git.openjdk.java.net/jdk/pull/2601
