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


Reply via email to