Uploaded image for project: 'Groovy'
  1. Groovy
  2. GROOVY-8229

nested try-catch-finally handling inside Closures generates wrong bytecode

    XMLWordPrintableJSON

Details

    • Bug
    • Status: Closed
    • Critical
    • Resolution: Fixed
    • 2.4.10, 2.4.11
    • 2.4.12
    • bytecode
    • None

    Description

      Since version 2.4.10 groovy generates the wrong bytecode instructions for try-catch-finally inside closures. This is most likely caused by one of these two changes: GROOVY-8085, GROOVY-7248.

      This was discovered with the generated code for Spocks verifyAll feature, here is the corresponding issue https://github.com/spockframework/spock/issues/709

      The problem can be observed without Spock in this scenario (@CompileStatic is only used to generate better readable decompiled code, the problem is the same with dynamic code).

      import groovy.transform.CompileStatic
      import org.junit.Assert
      import org.junit.Test
      import org.junit.runners.model.MultipleFailureException
      
      @CompileStatic
      class TryCatchProblem {
      
        @Test
        void "test"() {
          def cl = {
            ErrorCollector collector = new ErrorCollector()
            try {
              try {
                Assert.fail("a")
              } catch (Throwable e) {
                collector.collect(e)
              }
              try {
                Assert.fail("b")
              } catch (Throwable e) {
                collector.collect(e)
              }
              try {
                Assert.assertTrue("c", true)
              } catch (Throwable e) {
                collector.collect(e)
              }
            } finally {
              collector.check()
            }
          }
          cl()
        }
      }
      
      class ErrorCollector {
        List<Throwable> errors = []
        int ctr = 0
      
        void collect(Throwable t) {
          errors.add(t)
        }
      
        void check() {
          if (!errors.empty) {
            List<Throwable> x = new ArrayList<>(errors)
            x << new RuntimeException("ctr ${++ctr}")
            throw new MultipleFailureException(x)
          }
        }
      }
      

      The different results can be seen here: https://gist.github.com/leonard84/23f072b2c434b27e851a3e513570acf1

      Result-2.4.9

      java.lang.AssertionError: a
      java.lang.AssertionError: b
      java.lang.RuntimeException: ctr 1

      Result-2.4.10

      java.lang.AssertionError: a
      java.lang.AssertionError: b
      java.lang.AssertionError: a
      java.lang.AssertionError: b
      java.lang.RuntimeException: ctr 1
      java.lang.RuntimeException: ctr 3

      And the decompiled code can be seen here: https://gist.github.com/leonard84/1c91f8aebd0b8af599a6925404e5a11c

      The relevant part is here:

      2.4.9
                  try {
                      Assert.assertTrue("c", true);
                      final Object o = null;
                      collector.check();
                      return o;
                  }
                  catch (Throwable e3) {
                      collector.collect(e3);
                      final Object o2 = null;
                      collector.check();
                      return o2;
                  }
                  collector.check();
              }
              finally {
                  collector.check();
              }
              return null;
      }
      

      vs.

      2.4.10
                  Object o = null;
                  try {
                      Assert.assertTrue("c", true);
                      o = null;
                      try {
                          final ErrorCollector errorCollector = collector;
                          errorCollector.check();
                          return o;
                      }
                      catch (Throwable e3) {
                          collector.collect(e3);
                          final Object o3 = null;
                          collector.check();
                          return o3;
                      }
                  }
                  catch (Throwable t) {}
                  try {
                      final ErrorCollector errorCollector = collector;
                      errorCollector.check();
                      return o;
                  }
                  finally {}
                  collector.check();
              }
              finally {
                  collector.check();
              }
              return null;
      }
      

      In both cases too many calls to collector.check() are generated, however since the finally block was not executed in <groovy-2.4.10 it was not really an issue. In the 2.4.10 code the generated code is definitely wrong. Somehow a call to collector.check() was put in its own try-catch, which throws the first exception which is then catched, appended and thrown inside a new exception with the call to collector.check() inside the catch. This Exception is then overridden by the call to collector.check() in the finally block, as you can see by the ctr 3 instead of ctr 2 inside the second RuntimeException.

      Attachments

        Issue Links

          Activity

            People

              daniel_sun Daniel Sun
              leonard84 Leonard Brünings
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: