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

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

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: 2.4.10, 2.4.11
    • Fix Version/s: 2.4.12
    • Component/s: bytecode
    • Labels:
      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.

        Issue Links

          Activity

          Hide
          daniel_sun Daniel Sun added a comment -

          Hi Leonard,

          I verified the similiar code with 2.5.0-beta-1, failed to reproduce the try-catch-finally issue.

          try {
              try {
                  println 1
              } catch (e) {
                  println 2
              }
              
              try {
                  println 3
              } catch (e) {
                  println 4
              }
              
              try {
                  println 5
              } catch (e) {
                  println 6
              }
          } finally {
              println 7
          }
          
          /* yield
          1
          3
          5
          7
          
          */
          
          def c = {
              try {
                  try {
                      println 1
                  } catch (e) {
                      println 2
                  }
                  
                  try {
                      println 3
                  } catch (e) {
                      println 4
                  }
                  
                  try {
                      println 5
                  } catch (e) {
                      println 6
                  }
              } finally {
                  println 7
              }
          }
          
          c()
          
          /* yield
          1
          3
          5
          7
          
          */
          
          Show
          daniel_sun Daniel Sun added a comment - Hi Leonard, I verified the similiar code with 2.5.0-beta-1, failed to reproduce the try-catch-finally issue. try { try { println 1 } catch (e) { println 2 } try { println 3 } catch (e) { println 4 } try { println 5 } catch (e) { println 6 } } finally { println 7 } /* yield 1 3 5 7 */ def c = { try { try { println 1 } catch (e) { println 2 } try { println 3 } catch (e) { println 4 } try { println 5 } catch (e) { println 6 } } finally { println 7 } } c() /* yield 1 3 5 7 */
          Hide
          daniel_sun Daniel Sun added a comment -

          I guess it may be related to GROOVY-7248 after I completed another experiment:

            void collect(Throwable t) {
              errors.add(t)    // if the code is modified as the following, the issue is gone:      [].add(t)
            }
          

          Complete 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)    // if the code is modified as the following, the issue is gone:      [].add(t)
            }
          
            void check() {
              println '########################################################################'
              if (!errors.empty) {
                List<Throwable> x = new ArrayList<>(errors)
                x << new RuntimeException("ctr ${++ctr}")
                throw new MultipleFailureException(x)
              }
            }
          }
          
          Show
          daniel_sun Daniel Sun added a comment - I guess it may be related to GROOVY-7248 after I completed another experiment: void collect(Throwable t) { errors.add(t) // if the code is modified as the following, the issue is gone: [].add(t) } Complete 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) // if the code is modified as the following, the issue is gone: [].add(t) } void check() { println '########################################################################' if (!errors.empty) { List<Throwable> x = new ArrayList<>(errors) x << new RuntimeException( "ctr ${++ctr}" ) throw new MultipleFailureException(x) } } }
          Hide
          jwagenleitner John Wagenleitner added a comment -

          When running the test case provided in the description I first see the change in output (duplicate errors and incorrect count) starting at commit b549062485c for GROOVY-8085.

          Show
          jwagenleitner John Wagenleitner added a comment - When running the test case provided in the description I first see the change in output (duplicate errors and incorrect count) starting at commit b549062485c for GROOVY-8085 .
          Hide
          leonard84 Leonard Brünings added a comment -

          Hi Daniel Sun,

          regarding your statement:

            void collect(Throwable t) {
              errors.add(t)    // if the code is modified as the following, the issue is gone:      [].add(t)
            }
          

          Just so that I understand you correctly do you mean this?

          class ErrorCollector {
            List<Throwable> errors = []
            int ctr = 0
          
            void collect(Throwable t) {
              [].add(t)
            }
          
            void check() {
              if (!errors.empty) {
                List<Throwable> x = new ArrayList<>(errors)
                x << new RuntimeException("ctr ${++ctr}")
                throw new MultipleFailureException(x)
              }
            }
          }
          
          Show
          leonard84 Leonard Brünings added a comment - Hi Daniel Sun , regarding your statement: void collect(Throwable t) { errors.add(t) // if the code is modified as the following, the issue is gone: [].add(t) } Just so that I understand you correctly do you mean this? class ErrorCollector { List<Throwable> errors = [] int ctr = 0 void collect(Throwable t) { [].add(t) } void check() { if (!errors.empty) { List<Throwable> x = new ArrayList<>(errors) x << new RuntimeException( "ctr ${++ctr}" ) throw new MultipleFailureException(x) } } }
          Hide
          daniel_sun Daniel Sun added a comment - - edited

          Leonard, I've reproduce the issue via the simplifed code. If code throws exception in the finally block, the code will execute multi times...

          try {
              try {
                  println 1
              } catch (e) {
              }
          } finally {
              check()
          }
          
          def check() {
              println 7
              throw new RuntimeException("check failed...");
          }
          
          Show
          daniel_sun Daniel Sun added a comment - - edited Leonard, I've reproduce the issue via the simplifed code. If code throws exception in the finally block, the code will execute multi times... try { try { println 1 } catch (e) { } } finally { check() } def check() { println 7 throw new RuntimeException( "check failed..." ); }
          Hide
          leonard84 Leonard Brünings added a comment - - edited

          Daniel.Sun I could reproduce the behavior if either a closure or a non-void method is used.

            void tooManyChecks() {
              def cl = {
                try {
                  try {
                    println 1
                  } catch (e) {
                  }
                } finally {
                  check()
                }
              }
              cl()
            }
          
            def tooManyChecks2() {
              try {
                try {
                  println 1
                } catch (e) {
                }
              } finally {
                check()
              }
            }
          
            void thisWorksAsIntended() {
                try {
                  try {
                    println 1
                  } catch (e) {
                  }
                } finally {
                  check()
                }    
            }
          
            def check() {
              println 7
              throw new RuntimeException("check failed...");
            }
          

          tooManyChecks:
          1
          7
          7
          7

          tooManyChecks2:
          1
          7
          7
          7

          thisWorksAsIntended:
          1
          7

          I think that the return handling in conjunction to the finally changes produces this weird behavior.

          Show
          leonard84 Leonard Brünings added a comment - - edited Daniel.Sun I could reproduce the behavior if either a closure or a non-void method is used. void tooManyChecks() { def cl = { try { try { println 1 } catch (e) { } } finally { check() } } cl() } def tooManyChecks2() { try { try { println 1 } catch (e) { } } finally { check() } } void thisWorksAsIntended() { try { try { println 1 } catch (e) { } } finally { check() } } def check() { println 7 throw new RuntimeException( "check failed..." ); } tooManyChecks: 1 7 7 7 tooManyChecks2: 1 7 7 7 thisWorksAsIntended: 1 7 I think that the return handling in conjunction to the finally changes produces this weird behavior.
          Hide
          jwagenleitner John Wagenleitner added a comment - - edited

          Here's a test case I used, note that if you add a return after the check() call it succeeds. The finally blocks are being replayed by the recorder twice, even though GROOVY-8085 may have triggered the issue it may be due to some implicit return handling/closures somewhere else.

          class Groovy8229Bug extends GroovyTestCase {
          
              void testFinallyBlockInClosureCalledOnce() {
                  assert shouldFail(
                      '''
                          class TryCatchProblem {
                              static int count = 0
                              static void main(args) {
                                  def cl = {
                                      try {
                                          try {
                                              assert count == 0
                                          } catch (Throwable e) { }
                                      } finally {
                                          check()
                                      }
                                  }
                                  cl()
                              }
                              static void check() {
                                  throw new UnsupportedOperationException("check call count: ${++count}")
                              }
                          }
                      '''
                  ) == 'check call count: 1'
              }
          
          }
          

          Since GROOVY-7248 doesn't seem to be a cause or related, I'm removing it from the breaks link. If any disagrees please add it back.

          Show
          jwagenleitner John Wagenleitner added a comment - - edited Here's a test case I used, note that if you add a return after the check() call it succeeds. The finally blocks are being replayed by the recorder twice, even though GROOVY-8085 may have triggered the issue it may be due to some implicit return handling/closures somewhere else. class Groovy8229Bug extends GroovyTestCase { void testFinallyBlockInClosureCalledOnce() { assert shouldFail( ''' class TryCatchProblem { static int count = 0 static void main(args) { def cl = { try { try { assert count == 0 } catch (Throwable e) { } } finally { check() } } cl() } static void check() { throw new UnsupportedOperationException( "check call count: ${++count}" ) } } ''' ) == 'check call count: 1' } } Since GROOVY-7248 doesn't seem to be a cause or related, I'm removing it from the breaks link. If any disagrees please add it back.
          Hide
          daniel_sun Daniel Sun added a comment -

          John, I agree with you that the issue is not related with GROOVY-7249. I'm trying to fix it.

          Show
          daniel_sun Daniel Sun added a comment - John, I agree with you that the issue is not related with GROOVY-7249 . I'm trying to fix it.
          Show
          daniel_sun Daniel Sun added a comment - Fixed by https://github.com/apache/groovy/commit/0406787a5c29704800132dfde79f9a9018b3e27f
          Hide
          leonard84 Leonard Brünings added a comment -

          Thanks for the quick fix, will there be a fix in 2.4.x for this as well?

          Show
          leonard84 Leonard Brünings added a comment - Thanks for the quick fix, will there be a fix in 2.4.x for this as well?
          Hide
          daniel_sun Daniel Sun added a comment -

          My pleasure
          Sure, the fix will be merged into 2.x.x.

          Show
          daniel_sun Daniel Sun added a comment - My pleasure Sure, the fix will be merged into 2.x.x.

            People

            • Assignee:
              daniel_sun Daniel Sun
              Reporter:
              leonard84 Leonard Brünings
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development