RFR: 8156871: Possible concurrency issue with JVM_AddModuleExports

[Lois Foltan]

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