RFR JDK-8155005: java.lang.reflect.Module.WeakSet is not thread-safe

[Peter Levart]

Hi Alan,

I created an issue for this:

     JDK-8155005: java.lang.reflect.Module.WeakSet is not thread-safe

     https://bugs.openjdk.java.net/browse/JDK-8155005


I did what you suggested, renamed type parameters to <K1, K2, V>, split 
long line in Module and modified the test so that it now waits for entry 
to be expunged for up to 5 seconds when it is expected to be expunged 
but returns as soon as it detects that the entry is gone (usually in 1st 
100 ms). It still waits just 500 ms when the entry is expected to remain 
in the map. False negatives should be eliminated this way and the test 
still doesn't waist plenty of execution time.

Here's latest webrev:

http://cr.openjdk.java.net/~plevart/jdk9-dev/Module.WeakSet.multithreadUnsafe/webrev.04/


If this is accepted then I would need to know via which repo this should 
be pushed (jdk9/dev or jake).

Regards, Peter


On 04/24/2016 10:16 AM, Alan Bateman wrote:
>
> On 22/04/2016 14:42, Peter Levart wrote:
>> :
>>
>> I tried to reduce the complexity of WeakPairMap as much as I could. I 
>> added some docs that describe the architecture. Hopefully this is now 
>> easier to grasp:
>>
>> http://cr.openjdk.java.net/~plevart/jdk9-dev/Module.WeakSet.multithreadUnsafe/webrev.03/ 
>>
> I think this looks quite good.
>
> As there are two keys then what would you think about renaming the 
> type parameters to <K1, K2, V>?
>
> In the test when I wonder if 500ms is enough to guarantee that 
> references has been queued, esp. on loaded machines, maybe a previous 
> test has several something loop for example.
>
> In terms of the startup then only WeakPairMap should be loaded so I 
> think that is okay. We'll find that -XaddExports will be a bit more 
> expensive as will trigger quite a bit of initialization and class 
> loading but I think that is okay too.
>
> A minor comment on Module L518 is that we might want to split this 
> line to avoid it being too long. Alternatively, just replace 
> exportedToAll, exportedToOther and exportedToAllUnnamed with "exports".
>
>>
>> Another possibility for transientReads and transientExports (but not 
>> for transientUses) could be if each instance of Module object held a 
>> unique long id allocated at its creation from say AtomicLong. You 
>> could then construct maps with Long ids instead of Module(s), but 
>> that has a drawback that some Module (say a system module or a module 
>> of an app server) could accumulate ids from modules long gone (for 
>> example when some app in an app server is redeployed multiple times) 
>> so this would be a memory leak...
> I agree, particularly with exports (not reads, at least not anymore).
>
>>
>> a single global WeakPairMap has an advantage over individual 
>> WeakSet(s) per Module instance because it is accessed more frequently 
>> so expunging of stale entries happens more promptly.
> True. Another advantage is that it drops 3 instance fields.
>
> -Alan.