groovy

GroovyClassLoader script caching not working properly if scripts not in root dir

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 1.7.1
  • Fix Version/s: 1.9-beta-4, 1.8.4
  • Component/s: class generator
  • Labels:
    None
  • Environment:
    Windows Vista
  • Number of attachments :
    2

Description

GroovyClassLoader caches class files compiled from scripts not in root dir under keys not matching original name.

GroovyClassLoader loader = new GroovyClassLoader();
loader.addURL(myScriptRoot);

Class class1, class2;

try {
  class1 = loader.loadClass("test/script1");
  class2 = loader.loadClass("test/script1");
  assertSame(class1, class2);  // this fails
} catch (ClassNotFoundException e) {
  fail("Class node found!");
}

If I write as above, class is cached by GroovyClassLoader under key "test.script1" so looking it up under "test/script1" fails. If I try with loader.loadClass("test.script1") it will be cached under "test.test" which seems even worse.

Is there any workaround other than creating my own cache for compiled classes?

As a fix in the groovy code, we could move the line 

String className = name.replace('/', '.');

to the beginning of the loadClass(String, boolean, boolean, boolean) method and using it instead of name in the entire method. GroovyResourceLoader will handle dots fine and className will be proper cache key then.

This would fix looking up scripts with '/' as package separator. It is quite possible this is not recommended way of loading scripts anyway. To fix it for dot separated script paths the problem lies I believe here (GroovyClassLoader):

    protected Class recompile(URL source, String className, Class oldClass) throws CompilationFailedException, IOException {
        if (source != null) {
            // found a source, compile it if newer
            if ((oldClass != null && isSourceNewer(source, oldClass)) || (oldClass == null)) {
                sourceCache.remove(className);
                return parseClass(source.openStream(), className);
            }
        }
        return oldClass;
    }

parseClass second argument is fileName and we pass dot-separated className here. I would change it to
return parseClass(source.openStream(), className.replace('.','/'));

  1. Text File
    gcl.patch
    15/Dec/10 4:26 AM
    1 kB
    blackdrag blackdrag
  2. Text File
    test_case_and_fix_for_GROOVY-4529.patch
    11/Oct/11 10:45 AM
    3 kB
    Michal Rembiszewski

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Michal Rembiszewski added a comment -

Ops, sorry for not formatting source properly, I don't use Jira usually.

Show
Michal Rembiszewski added a comment - Ops, sorry for not formatting source properly, I don't use Jira usually.
Hide
Permalink
blackdrag blackdrag added a comment -

the normal contract for a ClassLoader is that you give to loadClass a class name. In your case you give a path. But the path is supposed to be added as root in addURL and then you would have had to call loadClass("script1") instead. What laodClass does is seeing the "/" as part of classname, but later interpreting it as path. This is surely not the best way, but since the method was used wrong form the beginning strange results are not really surprising.

If you want that, I suggest you use your own subclass of GroovyClassLoader with the path corrected to your liking.

Show
blackdrag blackdrag added a comment - the normal contract for a ClassLoader is that you give to loadClass a class name. In your case you give a path. But the path is supposed to be added as root in addURL and then you would have had to call loadClass("script1") instead. What laodClass does is seeing the "/" as part of classname, but later interpreting it as path. This is surely not the best way, but since the method was used wrong form the beginning strange results are not really surprising. If you want that, I suggest you use your own subclass of GroovyClassLoader with the path corrected to your liking.
Hide
Permalink
Michal Rembiszewski added a comment -

Thanks for your comment. So trying to load script through loadClass("package.script1") where "package" is dir under script root is also wrong? I have tried it and included such case in my report. It resulted in class with name "package.script1" being cached under name "package.package".

Show
Michal Rembiszewski added a comment - Thanks for your comment. So trying to load script through loadClass("package.script1") where "package" is dir under script root is also wrong? I have tried it and included such case in my report. It resulted in class with name "package.script1" being cached under name "package.package".
Hide
Permalink
blackdrag blackdrag added a comment -

if the script has a package statement defining it is in a package called "package", then it should have been right to load it through "package.script1", assuming there is a script1.groovy in a folder package, that is under one of the root folders. "package.package" is of course wrong. ive me afew minutes to verify that

Show
blackdrag blackdrag added a comment - if the script has a package statement defining it is in a package called "package", then it should have been right to load it through "package.script1", assuming there is a script1.groovy in a folder package, that is under one of the root folders. "package.package" is of course wrong. ive me afew minutes to verify that
Hide
Permalink
Michal Rembiszewski added a comment -

I attach the test case so it's less confusing, sorry for misleading example in the first place:

GroovyClassLoader loader = new GroovyClassLoader();
loader.addURL(myScriptRoot);

Class class1, class2;

try {
  class1 = loader.loadClass("test.script1");
  class2 = loader.loadClass("test.script1");
  assertSame(class1, class2);  // this also fails
} catch (ClassNotFoundException e) {
  fail("Class not found!");
}

The second fix in my original post should take care of that I believe. Intentional deny of package prefix when calling scripts would be a pity.

Show
Michal Rembiszewski added a comment - I attach the test case so it's less confusing, sorry for misleading example in the first place: GroovyClassLoader loader = new GroovyClassLoader(); loader.addURL(myScriptRoot); Class class1, class2; try { class1 = loader.loadClass( "test.script1" ); class2 = loader.loadClass( "test.script1" ); assertSame(class1, class2); // this also fails } catch (ClassNotFoundException e) { fail( " Class not found!" ); } The second fix in my original post should take care of that I believe. Intentional deny of package prefix when calling scripts would be a pity.
Hide
Permalink
blackdrag blackdrag added a comment -

may test showed, that if I put a script named script1 with the package foo into a folder foo in script1.groovy, add the parent of foo to the root urls and then load the script via loadClass("foo.script1"), then the class is loaded and cached correctly, as the second loading returns the same class.

Show
blackdrag blackdrag added a comment - may test showed, that if I put a script named script1 with the package foo into a folder foo in script1.groovy, add the parent of foo to the root urls and then load the script via loadClass("foo.script1"), then the class is loaded and cached correctly, as the second loading returns the same class.
Hide
Permalink
blackdrag blackdrag added a comment -

in other words your test passes for me on trunk

Show
blackdrag blackdrag added a comment - in other words your test passes for me on trunk
Hide
Permalink
blackdrag blackdrag added a comment -

ah wait, there might be a classpath problem and I may have executed the wrong test

Show
blackdrag blackdrag added a comment - ah wait, there might be a classpath problem and I may have executed the wrong test
Hide
Permalink
blackdrag blackdrag added a comment -

nope, all fine

Show
blackdrag blackdrag added a comment - nope, all fine
Hide
Permalink
Michal Rembiszewski added a comment -

Thank you very much for your time, I'll do some more investigation with trunk. This test passes for me when I do it for a class but fails in case of a script on versions 1.7.1-1.7.5. If it comes up with trunk, I'll prepare and attach proper test case.

Show
Michal Rembiszewski added a comment - Thank you very much for your time, I'll do some more investigation with trunk. This test passes for me when I do it for a class but fails in case of a script on versions 1.7.1-1.7.5. If it comes up with trunk, I'll prepare and attach proper test case.
Hide
Permalink
blackdrag blackdrag added a comment -

ok, just tested with a script and then it fails, so I now can confirm the problem

Show
blackdrag blackdrag added a comment - ok, just tested with a script and then it fails, so I now can confirm the problem
Hide
Permalink
Michal Rembiszewski added a comment -

Reopening the issue then, thanks again. The second part of the original post contains suggested solution.

Show
Michal Rembiszewski added a comment - Reopening the issue then, thanks again. The second part of the original post contains suggested solution.
Hide
Permalink
blackdrag blackdrag added a comment -

actually I am going to do a bit more. If the URL is a file I will use parseClass(File) and in case of the text I will add a dummy ".groovy" at the end.

Show
blackdrag blackdrag added a comment - actually I am going to do a bit more. If the URL is a file I will use parseClass(File) and in case of the text I will add a dummy ".groovy" at the end.
Hide
Permalink
blackdrag blackdrag added a comment -

I attached the fix as patch

Show
blackdrag blackdrag added a comment - I attached the fix as patch
Hide
Permalink
blackdrag blackdrag added a comment -

sadly the patch does fix the issue at hand, but does break other code. It seems generating that file name is causing a problem here for GroovyScriptEngine

Show
blackdrag blackdrag added a comment - sadly the patch does fix the issue at hand, but does break other code. It seems generating that file name is causing a problem here for GroovyScriptEngine
Hide
Permalink
Michal Rembiszewski added a comment -

Contains a fix passing all tests and two tests demonstrating the issue. Both problems are related and appear only for scripts (not classes) having non-empty packages. First one affects performance since such scripts are compiled every time they are called. The second one makes using log4j etc annoying since script classes have misleading names.

Show
Michal Rembiszewski added a comment - Contains a fix passing all tests and two tests demonstrating the issue. Both problems are related and appear only for scripts (not classes) having non-empty packages. First one affects performance since such scripts are compiled every time they are called. The second one makes using log4j etc annoying since script classes have misleading names.
Hide
Permalink
Michal Rembiszewski added a comment -

Patch is against trunk, made today (11 Oct 2011).

Show
Michal Rembiszewski added a comment - Patch is against trunk, made today (11 Oct 2011).

People

  • Assignee:
    Guillaume Laforge
    Reporter:
    Michal Rembiszewski
Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: