Thread in a Spring app that watches a directory for new files

Question

Before showing-off the code, here is a bit of an explanation.

I have a Spring component that spawns a thread for watching a directory for new files. In this context, I have a method that runs in a Java Thread and performs operations that might be interrupted (especially if the container class is being close()d). The Spring component is designed to be Autocloseable and dispose the thread when the component self is being disposed.

SonarCloud warns me that I catch the InterruptedException without re-throwing it, but I am just logging the event and returning.

public class MyBean implements MyInterface, InitializingBean, AutoCloseable {

    /**
     * Watcher thread
     * Listens for new files available in the input directory and notifies listeners synchronously
     */
    private Thread watcherThread;

    /**
     * File system watch key
     */
    private WatchKey fileSystemWatchKey;

    private WatchService watchService;


    @Override
    public synchronized void afterPropertiesSet() throws Exception {
            log.info("Watching directory {} for new files to acquire", inputPath);
            this.watchService = inputPath.getFileSystem().newWatchService();
            this.fileSystemWatchKey = inputPath.register(watchService, StandardWatchEventKinds.ENTRY_CREATE);

            watcherThread = new Thread(this::watchDirectory, beanName + "-watcher");
            watcherThread.setPriority(Thread.MIN_PRIORITY);
            watcherThread.setDaemon(true);
            watcherThread.setUncaughtExceptionHandler((thread, exception) -> log.error("Exception in watcher thread {}: {}", thread.getName(), exception.getMessage(), exception));
            watcherThread.start();

    }

    @Override
    public void close() {
        //Stop watcher thread
        if (watcherThread != null)
            try {
                log.debug("Stopping thread {}", watcherThread.getName());
                watcherThread.interrupt();
                watcherThread.join(60000);
                log.debug("Stopped thread {}", watcherThread.getName());
            } catch (InterruptedException ex) {
            } catch (Exception ex) {
                log.error("Error disposing watcher thread {}", watcherThread.getName(), ex);
            }
        watcherThread = null;
    }

    /**
     * Starts a watch loop over the file system
     */
    @SuppressWarnings("unchecked")
    protected void watchDirectory() {
        if (watchService == null)
            throw new IllegalStateException("Watch service not initialized");

        while (true) {
            try {
                WatchKey watchKey = watchService.take(); //This throws InterruptedException
                if (watchKey == null) {
                    log.debug("No new file found in input directory");
                    Thread.sleep(60000);
                    continue;
                }

                for (WatchEvent<?> pollEvent : watchKey.pollEvents()) {
                    WatchEvent.Kind<?> kind = pollEvent.kind();
                    if (!Path.class.isAssignableFrom(kind.type())) {
                        log.trace("Unsupported event kind {} type {}", kind.name(), kind.type());
                        continue;
                    }

                    WatchEvent<Path> pathWatchEvent = (WatchEvent<Path>) pollEvent;
                    File newFile = this.inputPath.resolve(pathWatchEvent.context()).toFile().getAbsoluteFile();
                    if (newFile.isFile())
                        try {
                            doStuff();
                        } finally {
                            doFinalize();
                        }
                }

            } catch (ClosedWatchServiceException closedWatchServiceException) {
                log.debug("Closed watch service. Ending loop");
                log.trace(closedWatchServiceException.getMessage(), closedWatchServiceException);
                return;
            } catch (InterruptedException ex) {
                log.debug("Watcher thread interrupted: ", getBeanName());
                log.trace(ex.getMessage(), ex);
                return;
            } catch (IOException ex) {
                log.error("I/O exception: {}", ex.getMessage(), ex);
                return;
            }

        }
    }

}

Notes:

• The bean is designed to be combination of InitializingBean/AutoCloseable: afterPropertiesSet starts the thread, close stops it
• Method WatchServie.take throws InterruptedException
• returning from thread method ends the thread. No other code to execute

Question: is it a false positive?

Offending fragment

        } catch (InterruptedException ex) {
            log.debug("Watcher thread interrupted: ", getBeanName());
            log.trace(ex.getMessage(), ex);
            return;
        }

Expected fragment

        } catch (InterruptedException ex) {
            log.debug("Watcher thread interrupted: ", getBeanName());
            log.trace(ex.getMessage(), ex);
            Thread.currentThread().interrupt();
            return;
        }

SonarCloud suggests me to add Thread.currentThread.interrupt(), but I don't find it meaningful in this context. Probably Sonar uses default rules that do not necessarily take all the context into consideration (this is why I suppressed a load more remarks in other contexts)

Answer 1

There is a major bug in your code. You use the Thread.interrupt() to control the thread lifecycle but you never check Thread.isInterrupted(). If close() is called when the thread is not waiting in Thread.sleep() or watchService.take(), the interrupt will be ignored and close will just wait a minute and exit as if everything went fine and the thread will go on. After this you log the thread exit as a fact without actually checking whether the thread finished or not.

The reason why Sonar suggests you to restore the status or rethrow the exception is described in depth here: https://stackoverflow.com/questions/6658196/why-restore-interrupt-flag-of-a-task-that-ran-in-a-thread-pools-thread . In your case it is not strictly necessary, but more of a common idiom that should be followed to ensure you don't miss it when you actually need to set it.

There is nothing you can do if the thread refuses to join due to the programming error or possibly other more complicated malfunction (the method does file IO so it could very well get blocked), so forcing the close() to wait a minute seems unnecessary. Is there a reason why close() absolutely has to wait until the thread has finished processing all the files? Can you remove the join() and make Thread.isInterrupted() an exit condition for both loops to make the thread quit it as soon as it is interrupted? This would release you from having to deal with InterruptedException in the close method. As the code is now, the empty one in the close() method is a valid issue of exception being ignored completely.

Always use curly braces in your if-statements. Especially if the content of if-statement is a multi-line try-catch block. The curly braces might just be the first defence between you and a multi-milliion dollar security breach.