[GROOVY-8229] nested try-catch-finally handling inside Closures generates wrong bytecode

• 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
• Assignee: Daniel Sun
• Reporter: Leonard Brünings
• Created: 13/Jun/17 16:35
• Updated: 11/Jul/17 00:23
• Resolved: 15/Jun/17 15:06

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

• is broken by: GROOVY-8085 Exception in "finally" not caught by outer "try"