RFR: 8156871: Possible concurrency issue with JVM_AddModuleExports

[Lois Foltan]

On 6/14/2016 6:39 PM, Lois Foltan wrote:
>
> On 6/14/2016 5:48 PM, Karen Kinnear wrote:
>> Lois,
>>
>> Is this also the case for PackageEntryTable::add_entry and 
>> locked_create_entry_or_null - that those
>> should not be happening at a safe point?
>> And ModuleEntryTable::create_unnamed_module, 
>> ModuleEntryTable::new_entry, ModuleEntryTable::add_entry,
>> ModuleEntryTable::locked_create_entry_or_null, finalize_javabase
>>
>> My mental model is that all of these are triggered by java API calls 
>> and therefore won’t run during a safe point.
>>
>> I don’t expect you to need a new test for the changes listed above, 
>> this is just a tighter assertion at each location, so same tests should
>> cover these.
>
> Yes, it is the case.  I was hesitant to make those changes under this 
> fix.  I certainly can if you prefer over opening a new bug to fix 
> those other instances.
> Lois

Hi Karen,

After some thought it makes more sense to go forward and consistently 
make these like changes in the same changeset.  Here is a new webrev 
covering all the methods you pointed out above.

http://cr.openjdk.java.net/~lfoltan/bug_jdk8156871.2/webrev/

Thanks again for the review!
Lois

>>
>> thanks,
>> Karen
>>
>>> On Jun 14, 2016, at 5:11 PM, Lois Foltan <lois.foltan at oracle.com> 
>>> wrote:
>>>
>>>
>>> On 6/14/2016 2:57 PM, Karen Kinnear wrote:
>>>> Lois,
>>>>
>>>> Thank you for the fix and the test.
>>>>
>>>> Couple of questions:
>>>> 1) packageEntry.hpp
>>>> - can you please modify comment “not exported” first line “the 
>>>> package has not been explicitly qualified to a particular module” …
>>>>     - maybe you could change this to say “the package does not have 
>>>> qualified or unqualified exports”. Then the qualified exports comment
>>>> takes the all unnamed into account.
>>> Thanks Karen for the review!  Made the change above.
>>>> 2) set_unqual_exported()
>>>> Do you really want assert_locked_or_safepoint(Module_lock)?
>>>> I could see reducing the export level at a safe point - e.g. if you 
>>>> unload a class loader and a module, then I could see reducing
>>>> the exports list or even turning it into null.
>>>> But with today’s design I was expecting that the following 
>>>> transitions could not happen at a safe point and only with the lock 
>>>> (i.e. transitions
>>>> that increase the export):
>>>>     add_qexport
>>>>     set_is_exported_allUnnamed (which currently acquires the lock)
>>>>     set_unqual_exported() (which currently acquires the lock)
>>>>     set_unqual_exported()
>>> I see your point, you are correct.  I have made that change not only 
>>> for PackageEntry::set_unqual_exported() but for 
>>> PackageEntry::add_qexport() as well.
>>> The only mechanism today for transitioning a package that has been 
>>> qualifiedly exported to be unqualifiedly exported, (widening its 
>>> exportability to all),  is via a call to JVM_AddModuleExportsToAll. 
>>> During a safepoint, even if all modules on a package's export list 
>>> die, that package is still considered qualifiedly exported.
>>>
>>> New webrev:
>>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8156871.1/webrev/
>>>
>>> Thanks,
>>> Lois
>>>
>>>> thanks,
>>>> Karen
>>>>
>>>>
>>>>> On Jun 10, 2016, at 10:25 AM, Lois Foltan <lois.foltan at oracle.com> 
>>>>> wrote:
>>>>>
>>>>> Hello,
>>>>>
>>>>> Please review the work to finalize a concurrency issue when 
>>>>> setting the exported state of a PackageEntry.  The work completed 
>>>>> in bug https://bugs.openjdk.java.net/browse/JDK-8152404 actually 
>>>>> had the side effect of fixing this bug as well.  Prior to 
>>>>> JDK-8152404, each PackageEntry determined its exported state via 
>>>>> two flags, one of which was a general _is_exported flag followed 
>>>>> by a another more specific state of exportability flag that 
>>>>> determined if the package was exported qualifiedly or not to a 
>>>>> given module. Checking these two flags, as the PackageEntry::is_* 
>>>>> methods used to do without the Module_lock, was problematic and 
>>>>> yielded a situation where a call to add a module to a 
>>>>> PackageEntry's qualified exported entry list failed because it was 
>>>>> determined that the package was unqualifiedly exported when it 
>>>>> really was not.
>>>>>
>>>>> To complete this fix, I have removed the is_unqual_exported call 
>>>>> prior to setting a PackageEntry's exported list.  The method 
>>>>> PackageEntry::set_exported determines what the current package 
>>>>> export state is and acts correctly.  Also, the test case has been 
>>>>> added in this webrev since it is a good stress test case for JVM 
>>>>> module support.
>>>>>
>>>>> Passes JPRT, java/lang, java/util RBT hotspot nightly.
>>>>>
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8156871
>>>>> Open webrev: 
>>>>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8156871/webrev/
>>>>>
>>>>> Thanks,
>>>>> Lois
>