1
\$\begingroup\$

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)

\$\endgroup\$
1

1 Answer 1

1
\$\begingroup\$

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.

\$\endgroup\$
3
  • \$\begingroup\$ Are you sure that Sonar is complaining because of my Thread.interrupt? Java code won't compile if I don't catch InterruptedException in a thread. I'll add the rule details right way. And your feedback about the close method is very appreciated as after years of Java coding I still haven't figured out the proper way to handle threads and interruptions \$\endgroup\$ May 12 at 12:16
  • \$\begingroup\$ I have rewritten parts of the answer to clear a few inaccuracies. \$\endgroup\$ May 13 at 9:16
  • 1
    \$\begingroup\$ Explanation: waiting for the thread to be completed is my own pattern of disposable resource. I like, as coding style, that whatever resource instantiated by a disposable object is correctly and ultimately finalized on close/dispose. That means, if the bean spawns a thread, not only close should ask the thread to stop, but "at the end" of close method the system should be in a state mostly like the previous, ie. with no additional thread. This reminds me of a very frequent Apache Tomcat warning about application that spawned a thread but "failed" to stop it. This is why I want to wait \$\endgroup\$ May 13 at 11:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.