RFR: 8156871: Possible concurrency issue with JVM_AddModuleExports

[Karen Kinnear]

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.

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
>