GroovyScriptEngine.loadScriptByName loads the wrong class when scripts contain class definitions

As far as I know, this is a known limitation - the script files cannot contain multiple classes (in your case a script class + another explicitly defined class) - because the caching maps the script path and a class in a cache entry.

FooScript.groovy is a sample script with a class definition. GseTest.groovy is a script which demonstrates the issue.

No, no. The script that you are running with GroovyScriptEngine is FooScript.groovy, so that is the one that gets cached, and it has multiple classes instead of one - Foo and the script_class (which is there due to println and assert statements).

To validate it, if you change FooScript.groovy to as below, you should not see the issue anymore.

class Foo {
	String bar() { "bar" }
	static main(args) {
		println "Running FooScript.groovy"
		assert new Foo().bar() == "bar" 
	}
}

Yes, I agree there are two classes, Foo and FooScript, but the bug I am reporting is that GSE IS trying to cache both of them. I found the problem by debugging through the GSE code and putting breakpoints on all the lines that did a scriptCache.put().

The GroovyScriptEngine#ScriptClassLoader.parseClass() method first puts the class FooScript into the cache, and then the class Foo. The problem is the both use the same key, so the ScriptCacheEntry for class Foo overwrites the the entry for the FooScript class. Thus, on the second call to GSE.run() the wrong class is obtained out of the cache, and since it doesn't have a main method it throws an exception.

I also agree that the workaround you provided would allow the simple example I provided to execute, but it doesn't solve the root problem. And it won't work in the more complex case of Groovlets, since the static main method would not have access to the various bindings, such as the request, response or params.

Another point I would like to make is that Scripts written in this manner worked fine in versions of Groovy prior to 1.7.0, and only started failing after the modifications to GSE were introduced in 1.7+.

I am aware of the caching related issues that you are mentioning. That was the reason I said to start with that the current dependency-checking caching logic has this known limitation (not sure about its documentation) when it comes to a script file having multiple classes - then it runs into those sort of entry caching issues.

The main() was not really a workaround suggestion - it was just to get it confirmed that when the file has just one class, there is no caching issue.

This (dependency-checking) caching logic was introduced only in 1.7.x onwards, so the 1.6.x is not affected by it.

Overall, the issue reported is acknowledged, but also known.

I wanted others to confirm if we want to keep this design with this known limitation or rework that caching logic.

What if the parseClass() code that adds scripts to the cache first checks to see if the script is already in the cache. If it is then it could ignore the class if it is dependecny Set of the existing cached class. This should allow only the script_class entry (FooScript) to be added to scriptCache.

So the old code:

for (Map.Entry<String,Set<String>> entry: cache.entrySet()) {
  String className = entry.getKey();
  Class clazz = getClassCacheEntry(className);
  if (clazz==null) continue;
                
  String entryName = getPath(clazz);
  Set<String> value = convertToPaths(entry.getValue()); 
  ScriptCacheEntry cacheEntry = new ScriptCacheEntry(clazz,time,value);
                scriptCache.put(entryName,cacheEntry);
}

Could Be replaced with something like:

for (Map.Entry<String,Set<String>> entry: cache.entrySet()) {
  String className = entry.getKey();
  Class clazz = getClassCacheEntry(className);
  if (clazz==null) continue;
                
  String entryName = getPath(clazz);
                
  if (scriptCache.containsKey(entryName)) {
    ScriptCacheEntry existingCacheEntry = scriptCache.get(entryName);
    String existingEntryClassName = existingCacheEntry.scriptClass.getCanonicalName();
    Set<String> deps = cache.get(existingEntryClassName);
    if (deps.contains(className)) continue;
  }
                
  Set<String> value = convertToPaths(entry.getValue()); 
  ScriptCacheEntry cacheEntry = new ScriptCacheEntry(clazz,time,value);
  scriptCache.put(entryName,cacheEntry);
}

This is my first time looking into GSE, so I'm not sure whether these changes will impact any other existing functionality, but they appear to fix my problem.

I think the code change is very specific to your requirement where you have one script_class and one regular class and class is in the dependency set of the script.

What if the script you load through GSE is as below:

class Foo {
    static main(args){
        println "Running Foo -> main()"
    }
}

class Bar { }

In this scenario, the patch fails with the same error due to same reason - it first puts an entry having class Foo and then replaces it by the entry having class Bar. So the first round (when the cache was empty) goes through and then the 2nd round fails because it has class Bar (which replaced class Foo).

Yep, you're right, I hadn't considered that case.

Guillaume made some changes, so I am not sure it is still the case, but originally the class we compile had itself as dependency. Still... maybe a stupid idea... how about: we keep a Set<String> outside of the for-loop containing the entryNames. If the entryName is in that set already, we skip the scriptCache update. If entryName is not in the Set yet, we update the scriptCache entry. That way the first class will be written into the cache only. There should be somewhere a test based on GCL that ensures the order of the classes... cannot remember what test it was.

@Jochen, I tried my last example on the latest trunk and still had the issue.

I tried the code change suggested by you but that does not help much.

Let's say I have FooScript.groovy having classes FooScript and Foo (as in the original example of this JIRA). Now in the loop dependency cache cache.entrySet() looks like

. So more changes will be needed at different places in GSE.

I agree... A first step would be to use a LinkedHashMap, but that alone might not be enough. Mabye the class should be extracted by other means, as it is done currently by GCL itself.

Jochen, I am attaching a patch for this issue for review.

• It takes care of the ordering issues - StringSetMap now has primary class nodes preserved in the order in which it recieves them from CompilationUnit. For a particular class node also order is preserved (the node itself was already added first and then the various classes/interfaces it uses).
  So we now get the dependencies correctly as {FooScript=[FooScript, Foo], Foo=[Foo]}

• Your earlier suggestion is also implemented so that if the script file contains multiple classes, the later ones don't overwrite the primary one added as the first one.

It fixes the issue reported here.

If you are ok, I will check it in so Doug can try out the snapshot and verify from his end.

Hi - any review comments?

Looks good to me.

Hi Doug. I have fixed this issue and tested from my end.

Could you also try the snapshot at http://snapshots.repository.codehaus.org/org/codehaus/groovy/groovy-all/1.7.3-SNAPSHOT/ from your end and let us know if the fix works for you?

Thanks.

I tested on my end and everything looked good. Thanks for all of your help Roshan!

You're welcome, Doug. Thanks for verifying the fix.

Fixed