RFR: 8156871: Possible concurrency issue with JVM_AddModuleExports

[David Holmes]

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

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