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

[Claes Redestad]

On 2014-10-22 23:35, Mandy Chung wrote:
>
> On 10/21/2014 6:43 AM, Claes Redestad wrote:
>>
>> http://cr.openjdk.java.net/~redestad/8060130/webrev.07/
>
> Looks good.

Thanks!

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

>
> GetSystemPackage.java test
>    This is great.  Thanks for adding the test.

Thanks!

>
> Can you break the long lines such as the call to 
> JarBuilder.addClassFile and runSubprocess. 

Sure. Cleaned it up a bit.

> Have you run this on windows?  I think you need to use File.separator 
> rather than "/" in the -Xbootclasspath/p argument.

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.

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

>
> In the verifyPackage, it throws Exception.  You can simply throw 
> RuntimeException that would avoid the need of the checked exception. 

Sure. I've restructured a bit to keep line lengths in check.

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

Some IDEs...

>
> Copyright year should be 2014.

Fixed.

http://cr.openjdk.java.net/~redestad/8060130/webrev.08

/Claes

>
>>
>>
>> I'd prefer sticking to the double-checked idiom here, unless you insist.
>>
>> I've cleaned up the code to avoid assignment in if-clauses, which 
>> according to
>> anonymous sources makes the code more readable. Perhaps this addresses
>> some of your concerns?
>>
>
> Looks okay.  Initialize manifest to EMPTY_MANIFEST would clean that a 
> little.
>
>>
>> Since I don't want to add binary JAR files, I opted to add a test 
>> which creates two JAR files,
>> each with a single class (with/without manifest) and then proceeds to 
>> spawn processes
>> to verify that:
>>
>
> Thanks for adding the test.
>
>> - when the JAR with manifest is on bootclasspath, the package can be 
>> found via
>> Package.getSystemPackage and the package object reflects values added 
>> to the manifest
>> - when the JAR without manifest is on bootclaspath, the package can 
>> be found via
>> Package.getSystemPackage but is empty apart from name
>> - adding the test.classes directory to bootclasspath behaves the same 
>> as adding the JAR
>> without manifest
>> - for any case where the class/package is not on the bootclasspath, 
>> the package information
>> can not be found via Package.getSystemPackage().
>>
>> Does this cover everything?
>>
>
> This is great.   See my comment above.
>
>> I guess there might be a way to make @run main/othervm or 
>> main/bootclasspath pick
>> up a dynamically generated JAR file, but I've failed to find a way 
>> that would make this work without
>> pregenerating the JARs. Suggestions on how this can be simplified are 
>> welcome.
>>
>
> What you have is fine.   The other way is to do it in a shell test 
> that is not preferable as you would have to manage FS for different 
> OSes etc.
>
> Mandy
>