RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package

[Mandy Chung]

On 10/22/2014 4:58 PM, Claes Redestad wrote:
>
>>
>> A minor nit: Package.java line 636:   it can return EMPTY_MANIFEST 
>> that will set manifest to non-null so that an invalid file would 
>> avoid opening the file every time getManifest is called (although 
>> this case rarely happens).  line 639 can then be simplified.
>
> We need to consider the case where JarInputStream.getManifest() 
> returns null, which would mean we'd either end up no simpler on line 
> 639 or simply adding a similar check around the value returned from 
> jis.getManifest(). I also think I saw that referencing the static 
> EMPTY_PACKAGES from within the privileged anonymous class adds 
> synthetic access methods...
>
> More importantly, the current approach should already ensure any file 
> is only scanned once by virtue of always setting manifest to a 
> non-null value in the end. No?

That's right.  I missed that you did set manifest to non-null in line 
639.  That's fine then.
>
>
> Yes, I've run the test via JPRT and verified it gets picked up and run 
> using the default testset, so Windows, Solaris, Mac and Linux should 
> be covered.

That's good.

>
>>
>> This test uses a special class loader that delegates to null class 
>> loader only.  Since you have the verification in place, it'd be good 
>> to also call Package.getPackage and Package.getPackages to verify 
>> that the same "package2" instance is returned.  As a sanity check, 
>> you could check "java.lang" package must be present.
>
> I've added some sanity checks as suggested. Sadly, it seems that since 
> the application classloader will define and load package2 first in 
> this test, there'll always be a Package defined in the 
> ClassLoader.pkgs that will mask the Package instance in Package.pkgs 
> when retrieved via the public methods in Package. 

Can you remove package2/Class2.class after you create the jar files?

> If JDK-8061804 is resolved, we could change to check for identity in 
> the Package.getPackages() case, which would improve the specificness 
> of the test... For now, perhaps we could trick things in this test and 
> put our dummy class into java.lang in our test here and ensure that 
> the package retrieved is identical. Worth the hassle?

What I meant is to check if the returned Package contains "java.lang" 
package that always exists in the system packages.  You don't need to 
put a dummy class in java.lang to get "java.lang" Package since it must 
be defined in the running VM.  e.g. you can refactor line 169-176 to a 
findPackage method that returns Package matching the given package name 
such that in line 164 you can call findPackage("java.lang") that should 
return non-null.


>
>> Nit: line 35-37, 43-46 - no need to declare these import as 
>> java.lang.* are imported by default.
>
> Some IDEs...
>

Which IDE are you using?  I think it might be your configuration.

Thanks
Mandy