[jira] [Created] (SLING-4477) JcrInstaller should not call Thread.interrupt()

["Thomas Mueller (JIRA)" <ji...@apache.org>]

Thomas Mueller created SLING-4477:
-------------------------------------

             Summary: JcrInstaller should not call Thread.interrupt()
                 Key: SLING-4477
                 URL: https://issues.apache.org/jira/browse/SLING-4477
             Project: Sling
          Issue Type: Improvement
          Components: Installer, JCR
            Reporter: Thomas Mueller


There JcrInstaller calls Thread.interrupt() where it's dangerous and not necessary. Thread.interrupt is dangerous because it closes files (when using the FileChannel API), including Lucene files, and the Oak persistent cache files. All further I/O operations with that file, including I/O operations on other threads, will then fail (see ClosedByInterruptException for details). OAK-2571 protects against closing persistent cache files, by reopening the files. But it results in slower performance and ugly log messages. 

Thread.interrupt is also dangerous because it does not work as expected if some code catches InterruptedException and does not re-throw it. See http://stackoverflow.com/questions/2020992/is-thread-interrupt-evil
Thread.interrupt is not necessary in most cases. Instead, a simple "volatile boolean" flag is sufficient, and much safer.

The JCR installer uses a boolean flag (active), but also Thread.interrupt, to stop the sleep period.

Just before the problem occurs, I see the following messages in the log file:

{noformat}
05.03.2015 15:22:26.642 *INFO* [CM Event Dispatcher (Fire ConfigurationEvent: pid=org.apache.sling.installer.provider.jcr.impl.JcrInstaller)] org.apache.sling.installer.provider.jcr.impl.JcrInstaller Deactivating Apache Sling JCR Installer
{noformat}

The very last message is from the org.apache.sling.installer.provider.jcr.impl.JcrInstaller, who calls:

{noformat}
backgroundThread.interrupt();
{noformat}

One possible solution is: in JcrInstaller, instead of:
{noformat}
        try {
            Thread.sleep(RUN_LOOP_DELAY_MSEC);
        } catch (final InterruptedException ignore) {
            // ignore
        }
{noformat}

use:
{noformat}
        synchronized (this) {
            if (active) {
                try {
                    wait(RUN_LOOP_DELAY_MSEC);
                } catch (final InterruptedException ignore) {
                    // ignore
                }
            }
        } 
{noformat}

and instead of:
{noformat}
        backgroundThread.interrupt();
{noformat}

use:
{noformat}
        synchronized (backgroundThread) {
            backgroundThread.notifyAll();
        } 
{noformat}

That's also better than what we have now, because right now, _any_ code within "runOneCycle" (including library and Oak code) that is doing "catch (InterruptedException x) { }" will let the "Thread.sleep(RUN_LOOP_DELAY_MSEC)" sleep one second too long.
And people catch and ignore InterruptedException a _lot_. Using wait and notifyAll does not suffer from this, and has no risk of trouble.

An alternative is to simply remove backgroundThread.interrupt(), and change the loop to:

{noformat}
        for (int i = 0; i < 100 && active; i++) {
            try {
                Thread.sleep(RUN_LOOP_DELAY_MSEC / 100);
            } catch (final InterruptedException ignore) {
                // ignore
            }
        }
{noformat}

This will let it sleep 10 ms too long at most. In this case, the field "active" needs to be volatile. But is needed even for the current code.




--
This message was sent by Atlassian JIRA
(v6.3.4#6332)