Uploaded image for project: 'Sling'
  1. Sling
  2. SLING-4477

JcrInstaller should not call Thread.interrupt()

    XMLWordPrintableJSON

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: JCR Installer 3.1.14
    • Component/s: Installer, JCR
    • Labels:
      None

      Description

      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:

      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
      

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

      backgroundThread.interrupt();
      

      One possible solution is: in JcrInstaller, instead of:

              try {
                  Thread.sleep(RUN_LOOP_DELAY_MSEC);
              } catch (final InterruptedException ignore) {
                  // ignore
              }
      

      use:

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

      and instead of:

              backgroundThread.active = false;
              logger.debug("Waiting for " + backgroundThread.getName() + " Thread to end...");
              backgroundThread.interrupt();
      

      use:

              synchronized (backgroundThread) {
                  backgroundThread.active = false;
                  backgroundThread.notifyAll();
              } 
              logger.debug("Waiting for " + backgroundThread.getName() + " Thread to end...");
      

      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:

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

      This will let it sleep 10 ms too long at most. It requires that RUN_LOOP_DELAY_MSEC is >= 100, which is ugly.

      In this case, the field "active" needs to be volatile. But is needed even for the current code (it's better to fix that in all cases).

        Attachments

          Activity

            People

            • Assignee:
              cziegeler Carsten Ziegeler
              Reporter:
              thomasm Thomas Mueller
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: