Github user anmolnar commented on a diff in the pull request:

    https://github.com/apache/zookeeper/pull/680#discussion_r239138642
  
    --- Diff: 
zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java ---
    @@ -446,4 +458,119 @@ private void configureSSLServerSocket(SSLServerSocket 
sslServerSocket) {
             LOG.debug("Using Java8-optimized cipher suites for Java version 
{}", javaVersion);
             return DEFAULT_CIPHERS_JAVA8;
         }
    +
    +    /**
    +     * Enables automatic reloading of the trust store and key store files 
when they change on disk.
    +     *
    +     * @throws IOException if creating the FileChangeWatcher objects fails.
    +     */
    +    public void enableCertFileReloading() throws IOException {
    +        LOG.info("enabling cert file reloading");
    +        ZKConfig config = new ZKConfig();
    +        String keyStoreLocation = 
config.getProperty(sslKeystoreLocationProperty);
    +        if (keyStoreLocation != null && !keyStoreLocation.isEmpty()) {
    +            final Path filePath = 
Paths.get(keyStoreLocation).toAbsolutePath();
    +            Path parentPath = filePath.getParent();
    +            if (parentPath == null) {
    +                throw new IOException(
    +                        "Key store path does not have a parent: " + 
filePath);
    +            }
    +            FileChangeWatcher newKeyStoreFileWatcher = new 
FileChangeWatcher(
    +                    parentPath,
    +                    watchEvent -> {
    +                        handleWatchEvent(filePath, watchEvent);
    +                    });
    +            // stop old watcher if there is one
    +            if (keyStoreFileWatcher != null) {
    +                keyStoreFileWatcher.stop();
    +            }
    +            keyStoreFileWatcher = newKeyStoreFileWatcher;
    +            keyStoreFileWatcher.start();
    +        }
    +        String trustStoreLocation = 
config.getProperty(sslTruststoreLocationProperty);
    +        if (trustStoreLocation != null && !trustStoreLocation.isEmpty()) {
    +            final Path filePath = 
Paths.get(trustStoreLocation).toAbsolutePath();
    +            Path parentPath = filePath.getParent();
    +            if (parentPath == null) {
    +                throw new IOException(
    +                        "Trust store path does not have a parent: " + 
filePath);
    +            }
    +            FileChangeWatcher newTrustStoreFileWatcher = new 
FileChangeWatcher(
    +                    parentPath,
    +                    watchEvent -> {
    +                        handleWatchEvent(filePath, watchEvent);
    +                    });
    +            // stop old watcher if there is one
    +            if (trustStoreFileWatcher != null) {
    +                trustStoreFileWatcher.stop();
    +            }
    +            trustStoreFileWatcher = newTrustStoreFileWatcher;
    +            trustStoreFileWatcher.start();
    +        }
    +    }
    +
    +    /**
    +     * Disables automatic reloading of the trust store and key store files 
when they change on disk.
    +     * Stops background threads and closes WatchService instances.
    +     */
    +    public void disableCertFileReloading() {
    +        if (keyStoreFileWatcher != null) {
    +            keyStoreFileWatcher.stop();
    +            keyStoreFileWatcher = null;
    +        }
    +        if (trustStoreFileWatcher != null) {
    +            trustStoreFileWatcher.stop();
    +            trustStoreFileWatcher = null;
    +        }
    +    }
    +
    +    // Finalizer guardian object, see Effective Java item 7
    +    // TODO: finalize() is deprecated starting with Java 10. This needs to 
be
    +    // replaced with an explicit shutdown call.
    +    @SuppressWarnings("unused")
    +    private final Object finalizerGuardian = new Object() {
    --- End diff --
    
    Reading the referenced literature about this, I believe it should be better 
to avoid using finalizer like this. We might even be better avoid using 
finalizers entirely. There's no guarantee when finalizer gets executed, not 
even guarantee to be executed at all, it has a huge performance penalty, etc.
    
    We should rather implement an explicit `close()` method with the 
`AutoClosable` interface and call it from QuorumPeer's shutdown() method.


---

Reply via email to