RFR: 8156871: Possible concurrency issue with JVM_AddModuleExports

[Karen Kinnear]

Lois,

Looks good.

Thank you for doing the more specific check - I think this will help catch issues in future.
Good catch David :-)

thanks,
Karen

> On Jun 15, 2016, at 7:33 AM, Lois Foltan <lois.foltan at oracle.com> wrote:
> 
> 
> On 6/14/2016 9:50 PM, David Holmes wrote:
>> Hi Lois,
>> 
>> lock->is_locked() simply reports whether any thread has locked it:
>> 
>> mutex.hpp:  bool is_locked() const { return _owner != NULL; }
>> 
>> You need:
>> 
>> lock->owned_by_self()
> Hi David,
> 
> Yes, thank you, good catch.
>    http://cr.openjdk.java.net/~lfoltan/bug_jdk8156871.3/webrev/
> 
> Lois
> 
>> 
>> Cheers,
>> David
>> 
>> 
>> 
>> On 15/06/2016 10:24 AM, Lois Foltan wrote:
>>> 
>>> 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
>>>> 
>>> 
>