Commons JCI
  1. Commons JCI
  2. JCI-53

Stack overflow on cross imports in commons-jci-janino

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 1.0
    • Fix Version/s: 1.1
    • Component/s: compiler janino
    • Labels:
      None

      Description

      A stack overflow occurs when trying to compile classes with cross references using commons-jci-janino, because an infinite recursion. The simple presence of cross reference import is enough to cause the problem.

      Example: the following classes will recreate the problem:

      package test;
      
      import static test.Func2.func2;
      
      public class Func1 {
         public static boolean func1() throws Exception {
             return true;
         }
      }
      
      package test;
      
      import static test.Func1.func1;
      
      public class Func2 {
         public static boolean func2() throws Exception {
             return true;
         }
      } 
      

      The exception stack is:

      java.lang.StackOverflowError
      	at org.codehaus.janino.Parser.parseAndExpression(Parser.java)
      	at org.codehaus.janino.Parser.parseExclusiveOrExpression(Parser.java)
      	at org.codehaus.janino.Parser.parseInclusiveOrExpression(Parser.java)
      	at org.codehaus.janino.Parser.parseConditionalAndExpression(Parser.java)
      	at org.codehaus.janino.Parser.parseConditionalOrExpression(Parser.java)
      	at org.codehaus.janino.Parser.parseConditionalExpression(Parser.java)
      	at org.codehaus.janino.Parser.parseAssignmentExpression(Parser.java)
      	at org.codehaus.janino.Parser.parseExpression(Parser.java)
      	at org.codehaus.janino.Parser.parseReturnStatement(Parser.java)
      	at org.codehaus.janino.Parser.parseStatement(Parser.java)
      	at org.codehaus.janino.Parser.parseBlockStatement(Parser.java)
      	at org.codehaus.janino.Parser.parseBlockStatements(Parser.java)
      	at org.codehaus.janino.Parser.parseBlock(Parser.java)
      	at org.codehaus.janino.Parser.parseMethodBody(Parser.java)
      	at org.codehaus.janino.Parser.parseMethodDeclarationRest(Parser.java)
      	at org.codehaus.janino.Parser.parseClassBodyDeclaration(Parser.java)
      	at org.codehaus.janino.Parser.parseClassBody(Parser.java)
      	at org.codehaus.janino.Parser.parseClassDeclarationRest(Parser.java)
      	at org.codehaus.janino.Parser.parsePackageMemberTypeDeclaration(Parser.java)
      	at org.codehaus.janino.Parser.parseCompilationUnit(Parser.java)
      	at org.drools.commons.jci.compilers.JaninoJavaCompiler$CompilingIClassLoader.findIClass(JaninoJavaCompiler.java:90)
      	at org.codehaus.janino.IClassLoader.loadIClass(IClassLoader.java)
      	at org.codehaus.janino.UnitCompiler.loadFullyQualifiedClass(UnitCompiler.java)
      	at org.codehaus.janino.UnitCompiler.import2(UnitCompiler.java)
      	at org.codehaus.janino.UnitCompiler.access$2(UnitCompiler.java)
      	at org.codehaus.janino.UnitCompiler$1.visitSingleStaticImportDeclaration(UnitCompiler.java)
      	at org.codehaus.janino.Java$CompilationUnit$SingleStaticImportDeclaration.accept(Java.java)
      	at org.codehaus.janino.UnitCompiler.<init>(UnitCompiler.java)
      	at org.drools.commons.jci.compilers.JaninoJavaCompiler$CompilingIClassLoader.findIClass(JaninoJavaCompiler.java:91)
      	at org.codehaus.janino.IClassLoader.loadIClass(IClassLoader.java)
      	at org.codehaus.janino.UnitCompiler.loadFullyQualifiedClass(UnitCompiler.java)
      	at org.codehaus.janino.UnitCompiler.import2(UnitCompiler.java)
      	at org.codehaus.janino.UnitCompiler.access$2(UnitCompiler.java)
      	at org.codehaus.janino.UnitCompiler$1.visitSingleStaticImportDeclaration(UnitCompiler.java)
      	at org.codehaus.janino.Java$CompilationUnit$SingleStaticImportDeclaration.accept(Java.java)
      	at org.codehaus.janino.UnitCompiler.<init>(UnitCompiler.java)
      	at org.drools.commons.jci.compilers.JaninoJavaCompiler$CompilingIClassLoader.findIClass(JaninoJavaCompiler.java:91)
      	at org.codehaus.janino.IClassLoader.loadIClass(IClassLoader.java)
      	at org.codehaus.janino.UnitCompiler.loadFullyQualifiedClass(UnitCompiler.java)
      	at org.codehaus.janino.UnitCompiler.import2(UnitCompiler.java)
      	at org.codehaus.janino.UnitCompiler.access$2(UnitCompiler.java)
      	at org.codehaus.janino.UnitCompiler$1.visitSingleStaticImportDeclaration(UnitCompiler.java)
      	at org.codehaus.janino.Java$CompilationUnit$SingleStaticImportDeclaration.accept(Java.java)
      	at org.codehaus.janino.UnitCompiler.<init>(UnitCompiler.java)
      	at org.drools.commons.jci.compilers.JaninoJavaCompiler$CompilingIClassLoader.findIClass(JaninoJavaCompiler.java:91)
      	at org.codehaus.janino.IClassLoader.loadIClass(IClassLoader.java)
      	at org.codehaus.janino.UnitCompiler.loadFullyQualifiedClass(UnitCompiler.java)
              (... and the loop continues ... )
      

      Please note that the problem does not occur when using commons-jci-eclipse or when using JANINO from command line:

      [etirelli@localhost test]$ ../janino-2.5.9/bin/janinoc test/*.java
      [etirelli@localhost test]$ 
      

      This problem is affecting JANINO support in the Drools project as described in ticket:

      http://jira.jboss.com/jira/browse/JBRULES-1013

      Thanks,
      Edson

        Activity

        Hide
        Torsten Curdt added a comment -

        Hm.... I can reproduce it with "testCrossReferenceCompilation" in AbstractCompilerTestCase but even after re-writing the whole janino implementation (basically using the what the janino CLI is using) we still get the StackOverflowError.

        http://svn.apache.org/repos/asf/commons/proper/jci/trunk/compilers/janino/src/main/java/org/apache/commons/jci/compilers/JaninoJavaCompiler.java

        I will need to check with Arno.

        Show
        Torsten Curdt added a comment - Hm.... I can reproduce it with "testCrossReferenceCompilation" in AbstractCompilerTestCase but even after re-writing the whole janino implementation (basically using the what the janino CLI is using) we still get the StackOverflowError. http://svn.apache.org/repos/asf/commons/proper/jci/trunk/compilers/janino/src/main/java/org/apache/commons/jci/compilers/JaninoJavaCompiler.java I will need to check with Arno.
        Hide
        Arno Unkrig added a comment -

        Arno speaking,

        hello there at APACHE.ORG... unfortunately I'm on vacation in italy these days, so don't be confused when I mix up Ys and Zs on this italian keyboard in this italian internet cafe.

        I will take a closer look at this when I get home. My quick analysis now is that there seems to be a bug in jci-janino. Unfortunately the link

        http://svn.apache.org/viewvc/jakarta/commons/proper/jci/trunk/

        that I found on http://commons.apache.org/jci/source-repository.html does not work, so I cannot look into the JCI source code. However JANINO's "Compiler" class does

        protected IClass findIClass(final String type) throws ClassNotFoundException {
        // ...

        // Check the already-parsed compilation units.
        for (int i = 0; i < Compiler.this.parsedCompilationUnits.size(); ++i) {
        UnitCompiler uc = (UnitCompiler) Compiler.this.parsedCompilationUnits.get;
        IClass res = uc.findClass(topLevelClassName);
        if (res != null) {
        if (!className.equals(topLevelClassName))

        { res = uc.findClass(className); if (res == null) return null; }

        this.defineIClass(res);
        return res;
        }
        }

        , and this stops the recursion. Maybe it is possible that JaninoJavaCompiler can be rewritten such that it uses org.codehaus.janino.Compiler instead of REIMPLEMENTING it.

        Torsten, maybe the hint above helps? If not, I will look into it on saturday.

        CU

        Arno

        Show
        Arno Unkrig added a comment - Arno speaking, hello there at APACHE.ORG... unfortunately I'm on vacation in italy these days, so don't be confused when I mix up Ys and Zs on this italian keyboard in this italian internet cafe. I will take a closer look at this when I get home. My quick analysis now is that there seems to be a bug in jci-janino. Unfortunately the link http://svn.apache.org/viewvc/jakarta/commons/proper/jci/trunk/ that I found on http://commons.apache.org/jci/source-repository.html does not work, so I cannot look into the JCI source code. However JANINO's "Compiler" class does protected IClass findIClass(final String type) throws ClassNotFoundException { // ... // Check the already-parsed compilation units. for (int i = 0; i < Compiler.this.parsedCompilationUnits.size(); ++i) { UnitCompiler uc = (UnitCompiler) Compiler.this.parsedCompilationUnits.get ; IClass res = uc.findClass(topLevelClassName); if (res != null) { if (!className.equals(topLevelClassName)) { res = uc.findClass(className); if (res == null) return null; } this.defineIClass(res); return res; } } , and this stops the recursion. Maybe it is possible that JaninoJavaCompiler can be rewritten such that it uses org.codehaus.janino.Compiler instead of REIMPLEMENTING it. Torsten, maybe the hint above helps? If not, I will look into it on saturday. CU Arno
        Hide
        Torsten Curdt added a comment -

        Hey Arno, please see the link I provided above. It has the correct URL to the JCI implementation.
        I also already changed the Compiler to re-use the Janino classes (namely CachingJavaSourceClassLoader) ...which is why I am so puzzled now to still be seeing it. Hope to hear from you on Saturday then.

        Show
        Torsten Curdt added a comment - Hey Arno, please see the link I provided above. It has the correct URL to the JCI implementation. I also already changed the Compiler to re-use the Janino classes (namely CachingJavaSourceClassLoader) ...which is why I am so puzzled now to still be seeing it. Hope to hear from you on Saturday then.
        Hide
        Arno Unkrig added a comment -

        Hi Torsten,

        I analyzed the JCI code, and there are two important results:

        (1)
        Yes, it's a bug in org.codehaus.janino.UnitCompiler, and I fixed it. Find the modified source attached, please test. When your tests are successful, then I will release the fix quickly as part of the next JANINO release.

        (2)
        You should definitely use the org.codehaus.janino.Compiler in favor of CachingJavaSourceClassLoader. The latter is NOT intended to serve as a Java compiler; instead, it stores "some" data in the persistent cache that you provider. These data may be class files, but they need not necessarily. Particularly, the data need not be the COMPLETE class files generated by the CJSCL. Rewriting JaninoJavaCompiler to use Compiler instead of CJSCL should be pretty much straightforward... if you have problems, I will help.

        Notice: The bug you identified affects both Compiler and CJSCL.

        CU

        Arno

        Show
        Arno Unkrig added a comment - Hi Torsten, I analyzed the JCI code, and there are two important results: (1) Yes, it's a bug in org.codehaus.janino.UnitCompiler, and I fixed it. Find the modified source attached, please test. When your tests are successful, then I will release the fix quickly as part of the next JANINO release. (2) You should definitely use the org.codehaus.janino.Compiler in favor of CachingJavaSourceClassLoader. The latter is NOT intended to serve as a Java compiler; instead, it stores "some" data in the persistent cache that you provider. These data may be class files, but they need not necessarily. Particularly, the data need not be the COMPLETE class files generated by the CJSCL. Rewriting JaninoJavaCompiler to use Compiler instead of CJSCL should be pretty much straightforward... if you have problems, I will help. Notice: The bug you identified affects both Compiler and CJSCL. CU Arno
        Show
        Arno Unkrig added a comment - http://fisheye.codehaus.org/browse/~raw,r=1.53/janino/janino/src/org/codehaus/janino/UnitCompiler.java
        Hide
        Torsten Curdt added a comment -

        Arno, that's great news. Unfortunately the link is broken for me. And at

        http://fisheye.codehaus.org/changelog/janino/janino

        I cannot see any changes committed since 30 July 2007. What am I missing?

        Show
        Torsten Curdt added a comment - Arno, that's great news. Unfortunately the link is broken for me. And at http://fisheye.codehaus.org/changelog/janino/janino I cannot see any changes committed since 30 July 2007. What am I missing?
        Hide
        Torsten Curdt added a comment -

        The new janino release fixed the problem.

        Show
        Torsten Curdt added a comment - The new janino release fixed the problem.
        Hide
        Arno Unkrig added a comment -

        Hi Torsten,

        CODEHAUS's FISHEYE seems to be out-of-date: E.g. it reports UnitCompiler's commit on 2007-07-30, but ever since I have committed 1.53 (2007-09-01) and 1.54 (2007-09-01).

        I posted a report on CODEHAUS chores: http://jira.codehaus.org/browse/HAUS-1569

        CU

        Arno

        Show
        Arno Unkrig added a comment - Hi Torsten, CODEHAUS's FISHEYE seems to be out-of-date: E.g. it reports UnitCompiler's commit on 2007-07-30, but ever since I have committed 1.53 (2007-09-01) and 1.54 (2007-09-01). I posted a report on CODEHAUS chores: http://jira.codehaus.org/browse/HAUS-1569 CU Arno
        Hide
        Mark Proctor added a comment -

        the current JCI implementation is not correctly handling all problems, for instance ParseExceptions are being thrown up and not placed into the problems array. I've since added this to the compiler.compile method, but I can't help feeling this should be handled elsewhere. Am I correct in assuming that IOException and CompileException are correctly handled, what about ScanException, is that thrown too without being added to the problems array.

        This is what I've added to JaninoJavaCompiler for now.

        try

        { compiler.compile(resources); }

        catch ( ScanException e )

        { problems.add(new JaninoCompilationProblem(e)); // is this already handled? }

        catch ( ParseException e )

        { problems.add(new JaninoCompilationProblem(e)); // we know this isn't handled }

        catch ( IOException e )

        { // I'm hoping the existing compiler problems handler catches these }

        catch ( CompileException e )

        { // I'm hoping the existing compiler problems handler catches these }
        Show
        Mark Proctor added a comment - the current JCI implementation is not correctly handling all problems, for instance ParseExceptions are being thrown up and not placed into the problems array. I've since added this to the compiler.compile method, but I can't help feeling this should be handled elsewhere. Am I correct in assuming that IOException and CompileException are correctly handled, what about ScanException, is that thrown too without being added to the problems array. This is what I've added to JaninoJavaCompiler for now. try { compiler.compile(resources); } catch ( ScanException e ) { problems.add(new JaninoCompilationProblem(e)); // is this already handled? } catch ( ParseException e ) { problems.add(new JaninoCompilationProblem(e)); // we know this isn't handled } catch ( IOException e ) { // I'm hoping the existing compiler problems handler catches these } catch ( CompileException e ) { // I'm hoping the existing compiler problems handler catches these }
        Hide
        Arno Unkrig added a comment -

        Hi Mark,

        you should modify the slightly as follows:

        try

        { compiler.compile(resources); }

        catch (LocatedException e)

        { // Fatal scan / parse / compile errors. problems.add(new JaninoCompilationProblem(e)); }

        catch ( IOException e )

        { // Low-level problem reading or writing bytes. log.error("this error should have been cought before", e); }

        Notice that JANINO 2.5.11 is required, because CompileException now extends LocatedException. In other words: You can now catch ScanException, ParseException and CompileException with a single "catch LocatedException" clause.

        Notice that "compile()" still throws CompileExceptions, even if you have "sectCompileErrorHandler()". The reason being is that sometimes there are fatal conditions that prevent the continuation of the compilation.

        Hope that helps!

        CU

        Arno

        Show
        Arno Unkrig added a comment - Hi Mark, you should modify the slightly as follows: try { compiler.compile(resources); } catch (LocatedException e) { // Fatal scan / parse / compile errors. problems.add(new JaninoCompilationProblem(e)); } catch ( IOException e ) { // Low-level problem reading or writing bytes. log.error("this error should have been cought before", e); } Notice that JANINO 2.5.11 is required, because CompileException now extends LocatedException. In other words: You can now catch ScanException, ParseException and CompileException with a single "catch LocatedException" clause. Notice that "compile()" still throws CompileExceptions, even if you have "sectCompileErrorHandler()". The reason being is that sometimes there are fatal conditions that prevent the continuation of the compilation. Hope that helps! CU Arno
        Hide
        Torsten Curdt added a comment -

        I have done the changes ...but not committed yet.
        2.5.11 is not available through the maven repos yet.

        Show
        Torsten Curdt added a comment - I have done the changes ...but not committed yet. 2.5.11 is not available through the maven repos yet.
        Hide
        Arno Unkrig added a comment -

        Hi there,

        anyything else I need to do for this? Otherwise I'd close

        http://jira.codehaus.org/browse/JANINO-101

        CU

        Arno

        Show
        Arno Unkrig added a comment - Hi there, anyything else I need to do for this? Otherwise I'd close http://jira.codehaus.org/browse/JANINO-101 CU Arno
        Hide
        Matt Fowles added a comment -

        Sorry to bring this ticket back from the dead. Arno has been MIA for a while and I inherited the Janino project. What is the state of this bug? Are you waiting on an update to Janino or anything I can help with?

        Show
        Matt Fowles added a comment - Sorry to bring this ticket back from the dead. Arno has been MIA for a while and I inherited the Janino project. What is the state of this bug? Are you waiting on an update to Janino or anything I can help with?
        Hide
        Torsten Curdt added a comment -

        updated to latest janino,
        issue was turned into a testcase,
        tests are passing,
        closing the issue

        Show
        Torsten Curdt added a comment - updated to latest janino, issue was turned into a testcase, tests are passing, closing the issue

          People

          • Assignee:
            Torsten Curdt
            Reporter:
            Edson Tirelli
          • Votes:
            1 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development