RFR: 8156871: Possible concurrency issue with JVM_AddModuleExports

[Lois Foltan]

On 6/15/2016 8:03 AM, Karen Kinnear wrote:
> Lois,
>
> Looks good.
Thank you Karen!
Lois

>
> 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