Lucene - Core
  1. Lucene - Core
  2. LUCENE-3334

IOUtils.closeSafely should log suppressed Exceptions in stack trace of original Exception (a new feature of Java 7)

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.4, 4.0-ALPHA
    • Component/s: core/other
    • Labels:
    • Lucene Fields:
      New, Patch Available

      Description

      I was always against Java 6 support, as it brings no really helpful new features into Lucene. But there are several things that make life easier in coming Java 7 (hopefully on July 28th, 2011). One of those is simplier Exception handling and suppression on Closeable, called "Try-With-Resources" (see http://docs.google.com/View?id=ddv8ts74_3fs7483dp, by the way all Lucene classes support these semantics in Java 7 automatically, the cool try-code below would work e.g. for IndexWriter, TokenStreams,...).

      We already have this functionality in Lucene since adding the IOUtils.closeSafely() utility (which can be removed when Java 7 is the minimum requirement of Lucene - maybe in 10 years):

      try (Closeable a = new ...; Closeable b = new ...) {
        ... use Closeables ...
      } catch (Exception e) {
        dosomething;
        throw e;
      }
      

      This code will close a and b in an autogenerated finally block and supress any exception. This is identical to our IOUtils.closeSafely:

      Exception priorException = null;
      Closeable a,b;
      try (Closeable a = new ...; Closeable b = new ...) {
        a = new ...;
        b = new ...
        ... use Closeables ...
      } catch (Exception e) {
        priorException = e;
        dosomething;
      } finally {
        IOUtils.closeSafely(priorException, a, b);
      }
      

      So this means we have the same functionality without Java 7, but there is one thing that makes logging/debugging much nicer:
      The above Java 7 code also adds maybe suppressed Exceptions in those Closeables to the priorException, so when you print the stacktrace, it not only shows the stacktrace of the original Exception, it also prints all Exceptions that were suppressed to throw this Exception (all Closeable.close() failures):

      org.apache.lucene.util.TestIOUtils$TestException: BASE-EXCEPTION
          at org.apache.lucene.util.TestIOUtils.testSuppressedExceptions(TestIOUtils.java:61)
          at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
          at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
          at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
          at java.lang.reflect.Method.invoke(Method.java:601)
          at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:44)
          at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:15)
          at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:41)
          at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:20)
          at org.junit.rules.TestWatchman$1.evaluate(TestWatchman.java:48)
          at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:28)
          at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:31)
          at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:76)
          at org.apache.lucene.util.LuceneTestCase$LuceneTestCaseRunner.runChild(LuceneTestCase.java:1486)
          at org.apache.lucene.util.LuceneTestCase$LuceneTestCaseRunner.runChild(LuceneTestCase.java:1404)
          at org.junit.runners.ParentRunner$3.run(ParentRunner.java:193)
          at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:52)
          at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:191)
          at org.junit.runners.ParentRunner.access$000(ParentRunner.java:42)
          at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:184)
          at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:28)
          at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:31)
          at org.junit.runners.ParentRunner.run(ParentRunner.java:236)
          at junit.framework.JUnit4TestAdapter.run(JUnit4TestAdapter.java:39)
          at org.apache.tools.ant.taskdefs.optional.junit.JUnitTestRunner.run(JUnitTestRunner.java:420)
          at org.apache.tools.ant.taskdefs.optional.junit.JUnitTestRunner.launch(JUnitTestRunner.java:911)
          at org.apache.tools.ant.taskdefs.optional.junit.JUnitTestRunner.main(JUnitTestRunner.java:768)
          Suppressed: java.io.IOException: TEST-IO-EXCEPTION-1
                  at org.apache.lucene.util.TestIOUtils$BrokenCloseable.close(TestIOUtils.java:36)
                  at org.apache.lucene.util.IOUtils.closeSafely(IOUtils.java:58)
                  at org.apache.lucene.util.TestIOUtils.testSuppressedExceptions(TestIOUtils.java:62)
                  ... 26 more
          Suppressed: java.io.IOException: TEST-IO-EXCEPTION-2
                  at org.apache.lucene.util.TestIOUtils$BrokenCloseable.close(TestIOUtils.java:36)
                  at org.apache.lucene.util.IOUtils.closeSafely(IOUtils.java:58)
                  at org.apache.lucene.util.TestIOUtils.testSuppressedExceptions(TestIOUtils.java:62)
                  ... 26 more
      

      For this in Java 7 a new method was added to Throwable, that allows logging such suppressed Exceptions (it is called automatically by the synthetic bytecode emitted by javac). This patch simply adds this functionality conditionally to IOUtils, so it "registers" all suppressed Exceptions, if running on Java 7. This is done by reflection: once it looks for this method in Throwable.class and if found, it invokes it in closeSafely, so the exceptions thrown on Closeable.close() don't get lost.

      This makes debugging much easier and logs all problems that may occur.

      This patch does not change functionality or behaviour, it just adds more nformation to the stack trace in a Java-7-way (similar to the way how Java 1.4 added causes). It works here locally on Java 6 and Java 7, but only Java 7 gets the additional stack traces. For Java 6 nothing changes. Same for Java 5 (if we backport to 3.x).

      This would be our first Java 7 improvement (a minor one). Next would be NIO2... - but thats not easy to do with reflection only, so we have to wait 10 years

      1. LUCENE-3334.patch
        6 kB
        Uwe Schindler
      2. LUCENE-3334.patch
        7 kB
        Uwe Schindler
      3. LUCENE-3334.patch
        7 kB
        Uwe Schindler

        Activity

        Hide
        Uwe Schindler added a comment -

        Patch for trunk, also with testcase. In tests.verbose=true it prints all stack traces, so simply try with Java 6 and Java 7.

        Show
        Uwe Schindler added a comment - Patch for trunk, also with testcase. In tests.verbose=true it prints all stack traces, so simply try with Java 6 and Java 7.
        Hide
        Uwe Schindler added a comment -

        Small cleanups in test and reflection.

        Show
        Uwe Schindler added a comment - Small cleanups in test and reflection.
        Hide
        Hoss Man added a comment -

        Sweet!

        I am naively wondering if replacing this...

        if (SUPPRESS_METHOD != null && exception != null && suppressed != null) {
          ...
        }
        

        ...with this...

        if (SUPPRESS_METHOD != null) {
          if (exception != null && suppressed != null) {
            ...
          }
        }
        

        would help runtime bytecode optimizers (ie: HotSpot) to replace that method with an inline NOOP when that method doesn't exist?

        Show
        Hoss Man added a comment - Sweet! I am naively wondering if replacing this... if (SUPPRESS_METHOD != null && exception != null && suppressed != null ) { ... } ...with this... if (SUPPRESS_METHOD != null ) { if (exception != null && suppressed != null ) { ... } } would help runtime bytecode optimizers (ie: HotSpot) to replace that method with an inline NOOP when that method doesn't exist?
        Hide
        Robert Muir added a comment -

        Patch for trunk, also with testcase. In tests.verbose=true it prints all stack traces, so simply try with Java 6 and Java 7.

        But its not really a testcase: we don't run hudson on java 7, and because we aren't testing this version of java, I don't
        think we should conditionalize code to use java7-only features that are totally untested.

        So I'm totally against patches like this until this is addressed, sorry. Otherwise perhaps lucene is broken on java 7, and we don't even know until users tell us.

        Show
        Robert Muir added a comment - Patch for trunk, also with testcase. In tests.verbose=true it prints all stack traces, so simply try with Java 6 and Java 7. But its not really a testcase: we don't run hudson on java 7, and because we aren't testing this version of java, I don't think we should conditionalize code to use java7-only features that are totally untested. So I'm totally against patches like this until this is addressed, sorry. Otherwise perhaps lucene is broken on java 7, and we don't even know until users tell us.
        Show
        Uwe Schindler added a comment - http://www.freshports.org/java/openjdk7/
        Hide
        Robert Muir added a comment -

        sure, but until there is a hudson job running, testing this stuff on java7, I don't think we should commit patches like this.

        this is the same argument i had about java 5 support (still untested to this day), it applies to java 7 equally.

        Show
        Robert Muir added a comment - sure, but until there is a hudson job running, testing this stuff on java7, I don't think we should commit patches like this. this is the same argument i had about java 5 support (still untested to this day), it applies to java 7 equally.
        Hide
        Uwe Schindler added a comment - - edited

        I am working on it, because Java 7 will come out on July 28th, and I want to be prepared on that.

        I would suggest that we add 2 more jobs for half hourly, that set an environment variable, so hudson-setting.sh uses another JDK.

        By the way, javac 7.0 warns you when you compile code with -source 1.6 and you dont set another bootclasspath, something that missing since years (the problem with missing methods/classes used in your code, even if -source 1.6).

        Show
        Uwe Schindler added a comment - - edited I am working on it, because Java 7 will come out on July 28th, and I want to be prepared on that. I would suggest that we add 2 more jobs for half hourly, that set an environment variable, so hudson-setting.sh uses another JDK. By the way, javac 7.0 warns you when you compile code with -source 1.6 and you dont set another bootclasspath, something that missing since years (the problem with missing methods/classes used in your code, even if -source 1.6).
        Hide
        Uwe Schindler added a comment -

        ...haven't tested @Override -source 1.5 violations...

        Show
        Uwe Schindler added a comment - ...haven't tested @Override -source 1.5 violations...
        Hide
        Robert Muir added a comment -

        ...haven't tested @Override -source 1.5 violations...

        That would be really nice if this worked somehow...

        Show
        Robert Muir added a comment - ...haven't tested @Override -source 1.5 violations... That would be really nice if this worked somehow...
        Hide
        Uwe Schindler added a comment -

        would help runtime bytecode optimizers (ie: HotSpot) to replace that method with an inline NOOP when that method doesn't exist?

        This may be a good idea if this code is performance critical. In that case its only exceuted when Exceptions are thrown and all is already f*cked up , so no need to optimize. For me the current code is more readable.

        I will keep this issue open until Oracle has fixed its HotSpot bug hopefully before the release of Java 7 (LUCENE-3335) and then commit once we have the Java7-Tasks running on Jenkins.

        Show
        Uwe Schindler added a comment - would help runtime bytecode optimizers (ie: HotSpot) to replace that method with an inline NOOP when that method doesn't exist? This may be a good idea if this code is performance critical. In that case its only exceuted when Exceptions are thrown and all is already f*cked up , so no need to optimize. For me the current code is more readable. I will keep this issue open until Oracle has fixed its HotSpot bug hopefully before the release of Java 7 ( LUCENE-3335 ) and then commit once we have the Java7-Tasks running on Jenkins.
        Hide
        Uwe Schindler added a comment -

        Here my final patch.

        Show
        Uwe Schindler added a comment - Here my final patch.
        Hide
        Uwe Schindler added a comment -

        Committed trunk revision: 1150405
        Committed 3.x revision: 1150407

        Show
        Uwe Schindler added a comment - Committed trunk revision: 1150405 Committed 3.x revision: 1150407

          People

          • Assignee:
            Uwe Schindler
            Reporter:
            Uwe Schindler
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development