> 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 o ther 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 ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/2601/files - new: https://git.openjdk.java.net/jdk/pull/2601/files/93292008..5ddb3cf7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2601&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2601&range=03-04 Stats: 90 lines in 2 files changed: 57 ins; 4 del; 29 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
