FileAlternationMonitor#stop() does not stop the thread

[anthonyraymond]

The thread if FileAlterationMonitor wasn't stopped by the stop(int) method, which forbid application to shutdown unless all Thread are exited (if FileAlterationMonitor is part of a DI managed component).

This behavior conflict with the method javadoc @param stopInterval the amount of time in milliseconds to wait for the thread to finish.

Simple example to understand why i changed the code

    Thread t = new Thread(() -> {
        try {
            Thread.sleep(500000);
        } catch (final InterruptedException e) {
        }
    });
    t.start();
    t.join(50);
   // Ok, we reach this point until 500000ms are elapsed, but the thread is still alive.
   //   because Thread#join(int) does not kill the thread. And the thread remains alive.

    Thread t = new Thread(() -> {
        try {
            Thread.sleep(500000);
        } catch (final InterruptedException e) {
        }
    });
    t.start();
    t.join(50);
    t.interupt();
   // Thread is exited

In this case, we waited the given time BEFORE exiting the Thread, as described in the javadoc, and the Thread is now finished and killed.

[anthonyraymond]

Jira issue : https://issues.apache.org/jira/browse/IO-535

[coveralls]

Coverage increased (+0.02%) to 90.086% when pulling bf49922 on anthonyraymond:master into bfd83b0 on apache:master.

[PascalSchumacher]

Is swallowing an InterruptedException a really bad practice (at least according to Java Concurrency in Practice and https://www.ibm.com/developerworks/library/j-jtp05236/)?

[anthonyraymond]

Tell me if i'm wrong, because it is kind of obscure.

The FileAlterationMonitor embed a thread. But when we call stop() we are calling from another thread (most likely the main thread).
Isn't Thread.currentThread().interrupt(); going to stop the calling thread? In our case the main thread?

Now, i agree that catching an exception to do nothing sounds dumb. But in our case we want to stop the inner thread. So even if it is interupted while performing the join() is kind of a good news. It just stopped earlier than expected.

Am i correct?

[garydgregory]

Thank you for your PR @anthonyraymond

Regardless of the implementation of this fix, it would be great to provide a unit test that fails without the main patch. Otherwise, "confidence is low" and regression will happen.

[anthonyraymond]

Hi @garydgregory .
@SvetlinZarev was faster than i was: #58