java.lang.reflect.Module.WeakSet is not thread-safe

[Alan Bateman]

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.