Lucene - Core
  1. Lucene - Core
  2. LUCENE-2053

When thread is interrupted we should throw a clear exception

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.0
    • Component/s: core/index
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      This is the 3.0 followon from LUCENE-1573. We should throw a dedicated exception, not just RuntimeException.

      Recent discussion from java-dev "Thread.interrupt()" subject: http://www.lucidimagination.com/search/document/8423f9f0b085034e/thread_interrupt

      1. LUCENE-2053.patch
        4 kB
        Michael McCandless
      2. LUCENE-2053.patch
        23 kB
        Michael McCandless

        Activity

        Hide
        Uwe Schindler added a comment -

        Here are some names for Exceptions:

        • UnexpectedInterruptException extends RuntimeException
        • IndexerInterruptedException extends RuntimeException
        • IndexWriterInterruptedException extends RuntimeException
        • SmartInterruptedException (just a joke)

        Another possibility would be to use IOException as super class, this would enable users to catch it without changing code and they will correctly log the message and not fallback to default RuntimeException catch in JVM.

        The patch is easy, you have to just find all Thread.interrupt() calls or catch InterruptedException in the code.

        Show
        Uwe Schindler added a comment - Here are some names for Exceptions: UnexpectedInterruptException extends RuntimeException IndexerInterruptedException extends RuntimeException IndexWriterInterruptedException extends RuntimeException SmartInterruptedException (just a joke) Another possibility would be to use IOException as super class, this would enable users to catch it without changing code and they will correctly log the message and not fallback to default RuntimeException catch in JVM. The patch is easy, you have to just find all Thread.interrupt() calls or catch InterruptedException in the code.
        Hide
        Michael McCandless added a comment -

        How about ThreadInterruptedException? Maybe under oal.util?

        I wouldn't restrict the name/scope to indexing. Sure, maybe today you can't Thread.interrupt a Lucene search, but maybe in the future we'll allow that and then this same exception should be thrown.

        I think it's cleaner to extend RuntimeException. Right now (2.9) you get a RuntimeException, so extending IOException instead is more of a back compat break.

        I just wish InterruptedException had been non-checked from the get-go...

        Show
        Michael McCandless added a comment - How about ThreadInterruptedException? Maybe under oal.util? I wouldn't restrict the name/scope to indexing. Sure, maybe today you can't Thread.interrupt a Lucene search, but maybe in the future we'll allow that and then this same exception should be thrown. I think it's cleaner to extend RuntimeException. Right now (2.9) you get a RuntimeException, so extending IOException instead is more of a back compat break. I just wish InterruptedException had been non-checked from the get-go...
        Hide
        Uwe Schindler added a comment -

        So I think we go with RuntimeException.

        • ThreadInterruptedException
        • UnexpectedInterruptedException
        • UncheckedInterruptedException

        ...

        Show
        Uwe Schindler added a comment - So I think we go with RuntimeException. ThreadInterruptedException UnexpectedInterruptedException UncheckedInterruptedException ...
        Hide
        Michael McCandless added a comment -

        OK I'll go with ThreadInterruptedException! I'll wait a while just in case someone has a violent reaction to that name

        Show
        Michael McCandless added a comment - OK I'll go with ThreadInterruptedException! I'll wait a while just in case someone has a violent reaction to that name
        Hide
        Michael McCandless added a comment -

        Attached patch.

        Show
        Michael McCandless added a comment - Attached patch.
        Hide
        Michael McCandless added a comment -

        Reopening to address buggy intermittent test failure...

        Show
        Michael McCandless added a comment - Reopening to address buggy intermittent test failure...
        Hide
        Michael McCandless added a comment -

        Attached my current approach for fixing the test.

        Includes a silly workaround to not let an interrupted thread hit the class loader. Weird....

        Show
        Michael McCandless added a comment - Attached my current approach for fixing the test. Includes a silly workaround to not let an interrupted thread hit the class loader. Weird....
        Hide
        Uwe Schindler added a comment -

        Patch looks good, even I do not understand it completely.

        Show
        Uwe Schindler added a comment - Patch looks good, even I do not understand it completely.
        Hide
        Michael McCandless added a comment -

        OK I'll commit shortly.

        Basically, with the patch the test is now more careful: the main thread issues the interrupt, and then waits for the child thread to confirm it handled the exception. This way the main thread won't send another interrupt until the child thread is done handling the last one.

        Show
        Michael McCandless added a comment - OK I'll commit shortly. Basically, with the patch the test is now more careful: the main thread issues the interrupt, and then waits for the child thread to confirm it handled the exception. This way the main thread won't send another interrupt until the child thread is done handling the last one.
        Hide
        Michael McCandless added a comment -

        OK, trying again!

        Show
        Michael McCandless added a comment - OK, trying again!

          People

          • Assignee:
            Michael McCandless
            Reporter:
            Michael McCandless
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development