[GROOVY-1980] GSE non-optimal behaviour

• Type: Bug
• Status: Closed
• Priority: Minor
• Resolution: Fixed
• Affects Version/s: 1.1-beta-2
• Fix Version/s: 1.1-beta-3
• Assignee: Paul King
• Reporter: Paul King
• Created: 14/Jul/07 00:42
• Updated: 07/Apr/15 19:06
• Resolved: 14/Jul/07 02:08

Description

From the mailing list:

Hi,

While trying to understand why GSE was so slow, I discovered a few bugs there

1) GSE in some cases doesn't close the connection it opens in
getResourceConnection() and in updateCacheEntry(). I fixed the one in
updateCacheEntry but it could still be some instances where
getResourceConnection at groovyScriptConn.getInputStream(); where it is not
closed later on in the code.

2) loadByScriptName(String) should not be deprecated, only
loadByScriptName(String, ClassLoader)

3) loadByScriptName(String) should not use groovyLoader but
initGroovyLoader(getClass().getParentLoader())

This will increase the loading significantly since it will be able to reuse
the cached entries.

HTH

Pascal

--- ../../../../groovy-core/src/main/groovy/util/GroovyScriptEngine.java	2007-07-09 14:27:19.000000000 -0700
+++ ./GroovyScriptEngine.java	2007-07-09 15:17:04.000000000 -0700
@@ -18,6 +18,7 @@
 import groovy.lang.Binding;
 import groovy.lang.GroovyClassLoader;
 import groovy.lang.Script;
+import groovyjarjarantlr.TokenStreamIOException;
 
 import java.io.*;
 import java.net.MalformedURLException;
@@ -94,6 +95,7 @@
      */
     private void initGroovyLoader (final ClassLoader parentClassLoader) {
         if (groovyLoader == null || groovyLoader.getParent() != parentClassLoader) {
+        
             groovyLoader = 
                 (GroovyClassLoader) AccessController.doPrivileged(new PrivilegedAction() {
                     public Object run() {
@@ -149,6 +151,8 @@
         ResourceException se = null;
         for (int i = 0; i < roots.length; i++) {
             URL scriptURL = null;
+            InputStream in = null;
+            
             try {
                 scriptURL = new URL(roots[i], resourceName);
 
@@ -156,7 +160,7 @@
 
                 // Make sure we can open it, if we can't it doesn't exist.
                 // Could be very slow if there are any non-file:// URLs in there
-                groovyScriptConn.getInputStream();
+                in = groovyScriptConn.getInputStream();
 
                 break; // Now this is a bit unusual
 
@@ -174,7 +178,7 @@
                 } else {
                     se = new ResourceException(message, se);
                 }
-            }
+            } 
         }
 
         // If we didn't find anything, report on all the exceptions that occurred.
@@ -264,15 +268,16 @@
 
     /**
      * Get the class of the scriptName in question, so that you can instantiate Groovy objects with caching and reloading.
-     * Note: This method is deprecated because we should not use a different parentClassLoader
      * @param scriptName
      * @return the loaded scriptName as a compiled class
      * @throws ResourceException
      * @throws ScriptException
-     * @deprecated
+
      */
     public Class loadScriptByName(String scriptName) throws ResourceException, ScriptException {
-        return loadScriptByName( scriptName, getClass().getClassLoader ());
+        scriptName = scriptName.replace('.', File.separatorChar) + ".groovy";
+        ScriptCacheEntry entry = updateCacheEntry(scriptName);
+        return entry.scriptClass;
     }
 
 
@@ -287,10 +292,8 @@
      */
     public Class loadScriptByName(String scriptName, ClassLoader parentClassLoader)
             throws ResourceException, ScriptException {
-        scriptName = scriptName.replace('.', File.separatorChar) + ".groovy";
         initGroovyLoader (parentClassLoader);
-        ScriptCacheEntry entry = updateCacheEntry(scriptName);
-        return entry.scriptClass;
+        return loadScriptByName (scriptName);
     }
 
     /**
@@ -343,12 +346,21 @@
                 // Make a new entry
                 ScriptCacheEntry currentCacheEntry = new ScriptCacheEntry();
                 currentCacheEntryHolder.set(currentCacheEntry);
+                InputStream in = null;
+                
                 try {
-                    currentCacheEntry.scriptClass = groovyLoader.parseClass(groovyScriptConn.getInputStream(), scriptName);
+                    in = groovyScriptConn.getInputStream();
+                    currentCacheEntry.scriptClass = groovyLoader.parseClass(in, scriptName);
                 } catch (Exception e) {
                     throw new ScriptException("Could not parse scriptName: " + scriptName, e);
                 } finally{
                     currentCacheEntryHolder.set(null);
+
+                    try {
+                        in.close();
+                    } catch (IOException e) {
+                        // Do nothing: Just want to make sure it is closed
+                    }
                 }
                 
                 currentCacheEntry.lastModified = lastModified;