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

[Peter Levart]

Hi Claes,

Regarding CachedManifest I have one more comment. It's usually better to 
use null/zero as "uninitialized" state for lazy-initialized fields. You 
spare one (volatile) write and don't worry about unsafe publication 
which might see the Java-default value instead of the one assigned in 
initializer as "uninitialized". So summing-up all comments from previous 
mails I would write CachedManifest class as:

     private static class CachedManifest {
         static final Manifest NO_MANIFEST = new Manifest();
         final String fileName;
         private final URL url;
         private volatile Manifest manifest;

         CachedManifest(final String fileName) {
             this.fileName = fileName;
             this.url = AccessController.doPrivileged(new 
PrivilegedAction<URL>() {
                 public URL run() {
                     final File file = new File(fileName);
                     if (file.isFile()) {
                         try {
                             return ParseUtil.fileToEncodedURL(file);
                         } catch (MalformedURLException e) {
                         }
                     }
                     return null;
                 }
             });
         }

         public URL getURL() {
             return url;
         }

         public Manifest getManifest() {
             if (url == null) return null;
             Manifest m = manifest;
             if (m == null) {
                 synchronized (this) {
                     if ((m = manifest) == null) {
                         manifest = m = 
AccessController.doPrivileged(new PrivilegedAction<Manifest>() {
                             public Manifest run() {
                                 try (FileInputStream fis = new 
FileInputStream(fileName);
                                      JarInputStream jis = new 
JarInputStream(fis, false))
                                 {
                                     return jis.getManifest();
                                 } catch (IOException e) {
                                     return NO_MANIFEST;
                                 }
                             }
                         });
                     }
                 }
             }
             return (m == NO_MANIFEST) ? null : m;
         }
     }


What do you say?

Regards, Peter


On 10/13/2014 04:32 PM, Peter Levart wrote:
> On 10/12/2014 04:09 AM, Claes Redestad wrote:
>>
>>
>> http://cr.openjdk.java.net/~redestad/8060130/webrev.03/
>>
>
> Hi Claes,
>
> First, one more nit:
>
> 1635                 if (!map.containsKey(pkgName)) {
> 1636                     map.put(pkgName, pkg);
> 1637                 }
>
> ..could be written as:
>
> map.putIfAbsent(pkgName, pkg);
>
>
>
> And second, I re-read the code of CachedManifest and noticed the 
> following. You initialize the volatile instance field 'manifest' like 
> that:
>
>  609         private volatile Manifest manifest = EMPTY_MANIFEST;
>
>
> So in following code:
>
>  631         public Manifest getManifest() {
>  632             resolveManifest();
>  633             return manifest;
>  634         }
>  635
>  636         private void resolveManifest() {
>  637             if (manifest != null || url == null) {
>  638                 return;
>  639             }
>  640             synchronized(this) {
>  641                 if (manifest == null) {
>  642                     manifest = AccessController.doPrivileged(new 
> PrivilegedAction<Manifest>() {
>  643                         public Manifest run() {
>  644                             return loadManifest(fileName);
>  645                         }
>  646                     });
>  647                 }
>  648             }
>  649         }
>
>
>
> ... loadManifest() will never be executed. getManifest() will always 
> return EMPTY_MANIFEST.
>
> You probably wanted line 637 to be written as:
>
> if (manivest != EMPTY_MANIFEST || url == null) { ...
>
>
> Likewise in loadManifest(), wou write:
>
>  586     private static Manifest loadManifest(String fn) {
>  587         try (FileInputStream fis = new FileInputStream(fn);
>  588              JarInputStream jis = new JarInputStream(fis, false))
>  589         {
>  590             return jis.getManifest();
>  591         } catch (IOException e) {
>  592             return EMPTY_MANIFEST;
>  593         }
>  594     }
>
> ..but you probably wanted the line 592 to return 'null' instead.
>
>
> Regards, Peter
>