<i18n dev> RFR: 8273790: Potential cyclic dependencies between Gregorian and CalendarSystem [v2]

Jaikiran Pai jpai at openjdk.java.net
Sat Sep 25 03:52:54 UTC 2021

On Sat, 25 Sep 2021 03:38:24 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-8273790?
>> As noted in that issue, trying to class load `sun.util.calendar.CalendarSystem` and `sun.util.calendar.Gregorian` concurrently in separate threads can lead to a deadlock because of the cyclic nature of the code in their static initialization. More specifically, consider this case:
>>  - Thread T1 initiates a classload on the `sun.util.calendar.CalendarSystem`.
>>  - This gives T1 the implicit class init lock on `CalendarSystem`. 
>>  - Consider thread T2 which at the same time initiates a classload on `sun.util.calendar.Gregorian` class.
>>  - This gives T2 a implicit class init lock on `Gregorian`.
>>  - T1, still holding a lock on `CalendarSystem` attempts to load `Gregorian` since it wants to create a (singleton) instance of `Gregorian` and assign it to the `static final GREGORIAN_INSTANCE` member. Since T2 is holding a class init lock on `Gregorian`, T1 ends up waiting
>>  - T2 on the other hand is still loading the `Gregorian` class. `Gregorian` itself "is a" `CalendarSystem`, so during this loading of `Gregorian` class, T2 starts travelling up the class hierarchy and asks for a lock on `CalendarSystem`. However T1 is holding this lock and as a result T2 ends up waiting on T1 which is waiting on T2. That triggers this deadlock.
>> The linked JBS issue has a thread dump which shows this in action.
>> The commit here delays the instance creation of `Gregorian` by moving that instance creation logic from the static initialization of the `CalendarSystem` class, to the first call to `CalendarSystem#getGregorianCalendar()`. This prevents the `CalendarSystem` from needing a lock on `Gregorian` during its static init (of course, unless some code in this static init flow calls `CalendarSystem#getGregorianCalendar()`, in which case it is back to square one. I have verified, both manually and through the jtreg test, that the code in question doesn't have such calls)
>> A new jtreg test has been introduced to reproduce the issue and verify the fix. The test in addition to loading these 2 classes in question, also additionally loads a few other classes concurrently. These classes have specific static initialization which leads the calls to `CalendarSystem#getGregorianCalendar()` or `CalendarSystem#forName()`. Including these classes in the tests ensures that this deadlock hasn't "moved" to a different location. I have run multiple runs (approximately 25) of this test with the fix and I haven't seen it deadlock anymore.
> Jaikiran Pai has updated the pull request incrementally with two additional commits since the last revision:
>  - Minor test adjustments as suggested by Naoto
>  - use a holder instead of volatile, as suggested by Roger

Hello Roger,

> As an alternative, can the Gregorian Instance be moved to a nested (static) class.
That will delay initialization until it is needed. This "holder" pattern is used elsewhere to defer initialization and break cycles.

I did indeed have that in mind when I started work on this. That was something Chris Hegarty had suggested and we have used in a different (but similar) issue a while back[1]. I was however unsure if that's a common enough technique, so had started off with the volatile approach. I've now updated the PR to use the holder technique instead.

[1] https://github.com/openjdk/jdk/pull/2893#issuecomment-797539503


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

More information about the i18n-dev mailing list