Merge lp:~mmcm/akiban-server/script-invoker-pool into lp:~akiban-technologies/akiban-server/trunk

[Mike McMahon]

Clean up ScriptInvoker as mentioned in lp:~pbeaman/akiban-server/direct-param-handling/+merge/156711/comments/343411

Now distinguishes the invoker proper from the library from which it comes. There are still no cases where more than one routine comes from the same library (a functional calling convention has one and a direct module has zero), but the pool itself now supports it, pooling the big ScriptEngine/Invocable once for all.

[Nathan Williams (nwilliams)]

Looks plausible.

review: Approve

=== modified file 'src/main/java/com/akiban/server/service/restdml/DirectServiceImpl.java'
--- src/main/java/com/akiban/server/service/restdml/DirectServiceImpl.java	2013-04-11 00:14:28 +0000
+++ src/main/java/com/akiban/server/service/restdml/DirectServiceImpl.java	2013-04-11 23:35:24 +0000
@@ -62,7 +62,8 @@
 import com.akiban.server.service.restdml.EndpointMetadata.ParamMetadata;
 import com.akiban.server.service.restdml.EndpointMetadata.Tokenizer;
 import com.akiban.server.service.routines.RoutineLoader;
-import com.akiban.server.service.routines.ScriptInvoker;
+import com.akiban.server.service.routines.ScriptLibrary;
+import com.akiban.server.service.routines.ScriptPool;
 import com.akiban.server.service.security.SecurityService;
 import com.akiban.server.service.session.Session;
 import com.akiban.server.types3.Attribute;
@@ -394,9 +395,19 @@
 
         final Object[] args = createArgsArray(pathParams, queryParameters, content, cache, md);
 
-        final ScriptInvoker invoker = conn.getRoutineLoader()
-                .getScriptInvoker(conn.getSession(), new TableName(procName.getSchemaName(), md.routineName)).get();
-        Object result = invoker.invokeNamedFunction(md.function, args);
+        final ScriptPool<ScriptLibrary> libraryPool = conn.getRoutineLoader()
+            .getScriptLibrary(conn.getSession(), new TableName(procName.getSchemaName(),
+                                                               md.routineName));
+        final ScriptLibrary library = libraryPool.get();
+        boolean success = false;
+        Object result;
+        try {
+            result = library.invoke(md.function, args);
+            success = true;
+        }
+        finally {
+            libraryPool.put(library, success);
+        }
 
         switch (md.outParam.type) {
 
@@ -529,9 +540,11 @@
             for (final Routine routine : ais.getRoutines().values()) {
                 if (routine.getCallingConvention().equals(CallingConvention.SCRIPT_LIBRARY)
                         && routine.getDynamicResultSets() == 0 && routine.getParameters().isEmpty()) {
+                    final ScriptPool<ScriptLibrary> libraryPool = routineLoader.getScriptLibrary(session, routine.getName());
+                    final ScriptLibrary library = libraryPool.get();
+                    boolean success = false;
                     try {
-                        final ScriptInvoker invoker = routineLoader.getScriptInvoker(session, routine.getName()).get();
-                        invoker.invokeNamedFunction(DISTINGUISHED_REGISTRATION_METHOD_NAME,
+                        library.invoke(DISTINGUISHED_REGISTRATION_METHOD_NAME,
                                 new Object[] { new RestFunctionRegistrar() {
                                     @Override
                                     public void register(String specification) throws Exception {
@@ -539,9 +552,11 @@
                                                 .getTableName(), specification);
                                     }
                                 } });
+                        success = true;
                     } catch (ExternalRoutineInvocationException e) {
                         if (e.getCause() instanceof NoSuchMethodException) {
                             LOG.warn("Script library " + routine.getName() + " has no _register function");
+                            success = true;
                             return;
                         }
                         Throwable previous = e;
@@ -557,6 +572,8 @@
                         throw e;
                     } catch (Exception e) {
                         throw new RegistrationException(e);
+                    } finally {
+                        libraryPool.put(library, success);
                     }
                 }
             }
 
=== modified file 'src/main/java/com/akiban/server/service/routines/RoutineLoader.java'
--- src/main/java/com/akiban/server/service/routines/RoutineLoader.java	2013-03-22 20:05:57 +0000
+++ src/main/java/com/akiban/server/service/routines/RoutineLoader.java	2013-04-11 23:35:24 +0000
@@ -33,5 +33,6 @@
     public boolean isScriptLanguage(Session session, String language);
     public ScriptPool<ScriptEvaluator> getScriptEvaluator(Session session, TableName routineName);
     public ScriptPool<ScriptInvoker> getScriptInvoker(Session session, TableName routineName);
+    public ScriptPool<ScriptLibrary> getScriptLibrary(Session session, TableName routineName);
     public void unloadRoutine(Session session, TableName routineName);
 }
 
=== modified file 'src/main/java/com/akiban/server/service/routines/RoutineLoaderImpl.java'
--- src/main/java/com/akiban/server/service/routines/RoutineLoaderImpl.java	2013-03-22 20:05:57 +0000
+++ src/main/java/com/akiban/server/service/routines/RoutineLoaderImpl.java	2013-04-11 23:35:24 +0000
@@ -194,6 +194,11 @@
     }
 
     @Override
+    public ScriptPool<ScriptLibrary> getScriptLibrary(Session session, TableName routineName) {
+        return scripts.getScriptLibrary(session, routineName);
+    }
+
+    @Override
     public void unloadRoutine(Session session, TableName routineName) {
         synchronized (loadablePlans) {
             loadablePlans.remove(routineName);
 
=== modified file 'src/main/java/com/akiban/server/service/routines/ScriptCache.java'
--- src/main/java/com/akiban/server/service/routines/ScriptCache.java	2013-04-03 13:41:34 +0000
+++ src/main/java/com/akiban/server/service/routines/ScriptCache.java	2013-04-11 23:35:24 +0000
@@ -61,8 +61,12 @@
         return getEntry(session, routineName).getScriptEvaluator();
     }
 
+    public ScriptPool<ScriptLibrary> getScriptLibrary(Session session, TableName routineName) {
+        return getEntry(session, routineName).getScriptLibrary();
+    }
+
     public ScriptPool<ScriptInvoker> getScriptInvoker(Session session, TableName routineName) {
-        return getEntry(session, routineName).getScriptInvoker();
+        return getEntry(session, routineName).getScriptInvoker(this, session);
     }
 
     protected ScriptEngineManager getManager(Session session) {
@@ -85,23 +89,25 @@
         return entry;
     }
 
-    static class CacheEntry {
+    class CacheEntry {
         private TableName routineName;
         private String script;
+        private TableName libraryName;
         private String function;
         private ScriptEngineFactory factory;
         private String threading;
         private boolean invocable, compilable;
         private ScriptPool<ScriptEvaluator> sharedEvaluatorPool;
-        private ScriptPool<ScriptInvoker> sharedInvokerPool;
+        private ScriptPool<ScriptLibrary> sharedLibraryPool;
         private ScriptEngine spareEngine;
 
         public CacheEntry(Routine routine, ScriptEngine engine) {
             routineName = routine.getName();
             script = routine.getDefinition();
+            libraryName = routineName; // TODO: Until qualified EXTERNAL NAME supported.
             function = routine.getMethodName();
             factory = engine.getFactory();
-            threading = (String) factory.getParameter("THREADING");
+            threading = (String)factory.getParameter("THREADING");
             invocable = (engine instanceof Invocable);
             compilable = (engine instanceof Compilable);
             spareEngine = engine;
@@ -127,9 +133,8 @@
                 }
             }
 
-            // Otherwise, every caller gets a new pool which only has the scope
-            // of the
-            // prepared statement, etc.
+            // Otherwise, every caller gets a new pool which only has
+            // the scope of the prepared statement, etc.
             ScriptEngine engine;
             synchronized (this) {
                 engine = spareEngine;
@@ -149,42 +154,54 @@
             }
         }
 
-        public ScriptPool<ScriptInvoker> getScriptInvoker() {
-            // TODO - talk with Mike about having a CacheEntry without a function -
-            // needed for the LIBRARY param styl
-            assert invocable ; //&& (function != null);
-            // Can share if at multi-threaded (or stronger), since we are
-            // invoking
-            // the function.
-            if ("MULTITHREADED".equals(threading) || "THREAD-ISOLATED".equals(threading)
-                    || "STATELESS".equals(threading)) {
+        public ScriptPool<ScriptLibrary> getScriptLibrary() {
+            assert invocable;
+            // Can share if at multi-threaded (or stronger), since we
+            // are invoking the function.
+            if ("MULTITHREADED".equals(threading) || 
+                "THREAD-ISOLATED".equals(threading) || 
+                "STATELESS".equals(threading)) {
                 synchronized (this) {
-                    if (sharedInvokerPool == null) {
+                    if (sharedLibraryPool == null) {
                         ScriptEngine engine = spareEngine;
                         if (engine != null)
                             spareEngine = null;
                         else
                             engine = factory.getScriptEngine();
-                        ScriptInvoker invoker = new Invoker(routineName, engine, script, function);
-                        sharedInvokerPool = new SharedPool<>(invoker);
+                        ScriptLibrary library = new Library(routineName, engine, script);
+                        sharedLibraryPool = new SharedPool<>(library);
                     }
-                    return sharedInvokerPool;
+                    return sharedLibraryPool;
                 }
             }
 
-            // Otherwise, every caller gets a new pool which only has the scope
-            // of the
-            // prepared statement, etc.
+            // Otherwise, every caller gets a new pool which only has
+            // the scope of the prepared statement, etc.
             ScriptEngine engine;
             synchronized (this) {
                 engine = spareEngine;
                 if (engine != null)
                     spareEngine = null;
             }
-            Invoker invoker = null;
+            Library library = null;
             if (engine != null)
-                invoker = new Invoker(routineName, engine, script, function);
-            return new InvokerPool(routineName, factory, script, function, invoker);
+                library = new Library(routineName, engine, script);
+            return new LibraryPool(routineName, factory, script, library);
+        }
+
+        public ScriptPool<ScriptInvoker> getScriptInvoker(ScriptCache cache, Session session) {
+            assert invocable && (function != null);
+            ScriptPool<ScriptLibrary> libraryPool;
+            if (routineName.equals(libraryName)) {
+                libraryPool = getScriptLibrary();
+            }
+            else {
+                synchronized (this) {
+                    spareEngine = null;
+                }
+                libraryPool = cache.getScriptLibrary(session, libraryName);
+            }
+            return new InvokerPool(libraryPool, function);
         }
     }
 
@@ -277,18 +294,14 @@
         }
     }
 
-    static class InvokerPool extends BasePool<ScriptInvoker> {
-        private final String function;
-
-        public InvokerPool(TableName routineName, ScriptEngineFactory factory, String script, String function,
-                Invoker initial) {
+    static class LibraryPool extends BasePool<ScriptLibrary> {
+        public LibraryPool(TableName routineName, ScriptEngineFactory factory, String script, Library initial) {
             super(routineName, factory, script, initial);
-            this.function = function;
         }
 
         @Override
-        protected Invoker create() {
-            return new Invoker(routineName, factory.getScriptEngine(), script, function);
+        protected Library create() {
+            return new Library(routineName, factory.getScriptEngine(), script);
         }
     }
 
@@ -396,14 +409,12 @@
         }
     }
 
-    static class Invoker implements ScriptInvoker {
+    static class Library implements ScriptLibrary {
         private final TableName routineName;
-        private final String function;
         private final Invocable invocable;
 
-        public Invoker(TableName routineName, ScriptEngine engine, String script, String function) {
+        public Library(TableName routineName, ScriptEngine engine, String script) {
             this.routineName = routineName;
-            this.function = function;
             setScriptName(routineName, engine);
             try {
                 if (engine instanceof Compilable) {
@@ -420,42 +431,70 @@
         }
 
         @Override
+        public String getEngineName() {
+            return ((ScriptEngine) invocable).getFactory().getEngineName();
+        }
+
+        @Override
+        public boolean isCompiled() {
+            return (invocable instanceof Compilable);
+        }
+
+        @Override
+        public Object invoke(String function, Object[] args) {
+            logger.debug("Calling {} in {}", function, routineName);
+            try {
+                return invocable.invokeFunction(function, args);
+            } catch (ScriptException ex) {
+                throw new ExternalRoutineInvocationException(routineName, ex);
+            } catch (NoSuchMethodException ex) {
+                throw new ExternalRoutineInvocationException(routineName, ex);
+            }
+        }
+    }
+
+    static class InvokerPool implements ScriptPool<ScriptInvoker> {
+        private final ScriptPool<ScriptLibrary> libraryPool;
+        private final String function;
+
+        public InvokerPool(ScriptPool<ScriptLibrary> libraryPool, String function) {
+            this.libraryPool = libraryPool;
+            this.function = function;
+        }
+
+        @Override
+        public ScriptInvoker get() {
+            return new Invoker(libraryPool.get(), function);
+        }
+
+        @Override
+        public void put(ScriptInvoker elem, boolean success) {
+            libraryPool.put(elem.getLibrary(), success);
+        }
+    }
+
+    static class Invoker implements ScriptInvoker {
+        private final ScriptLibrary library;
+        private final String function;
+
+        public Invoker(ScriptLibrary library, String function) {
+            this.library = library;
+            this.function = function;
+        }
+
+        @Override
+        public ScriptLibrary getLibrary() {
+            return library;
+        }
+
+        @Override
         public String getFunctionName() {
             return function;
         }
 
         @Override
-        public String getEngineName() {
-            return ((ScriptEngine) invocable).getFactory().getEngineName();
-        }
-
-        @Override
-        public boolean isCompiled() {
-            return (invocable instanceof Compilable);
-        }
-
-        @Override
         public Object invoke(Object[] args) {
-            logger.debug("Calling {} in {}", function, routineName);
-            try {
-                return invocable.invokeFunction(function, args);
-            } catch (ScriptException ex) {
-                throw new ExternalRoutineInvocationException(routineName, ex);
-            } catch (NoSuchMethodException ex) {
-                throw new ExternalRoutineInvocationException(routineName, ex);
-            }
-        }
-        
-        @Override
-        public Object invokeNamedFunction(String functionName, Object[] args) {
-            logger.debug("Calling {} in {}", function, routineName);
-            try {
-                return invocable.invokeFunction(functionName, args);
-            } catch (ScriptException ex) {
-                throw new ExternalRoutineInvocationException(routineName, ex);
-            } catch (NoSuchMethodException ex) {
-                throw new ExternalRoutineInvocationException(routineName, ex);
-            }
+            return library.invoke(function, args);
         }
     }
 }
 
=== modified file 'src/main/java/com/akiban/server/service/routines/ScriptInvoker.java'
--- src/main/java/com/akiban/server/service/routines/ScriptInvoker.java	2013-03-28 21:05:55 +0000
+++ src/main/java/com/akiban/server/service/routines/ScriptInvoker.java	2013-04-11 23:35:24 +0000
@@ -19,9 +19,7 @@
 
 public interface ScriptInvoker
 {
-    public String getEngineName();
+    public ScriptLibrary getLibrary();
     public String getFunctionName();
-    public boolean isCompiled();
     public Object invoke(Object[] args);
-    public Object invokeNamedFunction(String functionName, Object[] args);
 }
 
=== added file 'src/main/java/com/akiban/server/service/routines/ScriptLibrary.java'
--- src/main/java/com/akiban/server/service/routines/ScriptLibrary.java	1970-01-01 00:00:00 +0000
+++ src/main/java/com/akiban/server/service/routines/ScriptLibrary.java	2013-04-11 23:35:24 +0000
@@ -0,0 +1,25 @@
+/**
+ * Copyright (C) 2009-2013 Akiban Technologies, Inc.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+package com.akiban.server.service.routines;
+
+public interface ScriptLibrary
+{
+    public String getEngineName();
+    public boolean isCompiled();
+    public Object invoke(String function, Object[] args);
+}
 
=== modified file 'src/main/java/com/akiban/sql/script/ScriptFunctionJavaRoutine.java'
--- src/main/java/com/akiban/sql/script/ScriptFunctionJavaRoutine.java	2013-03-22 20:05:57 +0000
+++ src/main/java/com/akiban/sql/script/ScriptFunctionJavaRoutine.java	2013-04-11 23:35:24 +0000
@@ -32,6 +32,7 @@
 import com.akiban.server.explain.Label;
 import com.akiban.server.explain.PrimitiveExplainer;
 import com.akiban.server.service.routines.ScriptInvoker;
+import com.akiban.server.service.routines.ScriptLibrary;
 import com.akiban.server.service.routines.ScriptPool;
 
 import java.sql.ResultSet;
@@ -129,11 +130,12 @@
     public CompoundExplainer getExplainer(ExplainContext context) {
         Attributes atts = new Attributes();
         ScriptInvoker invoker = pool.get();
+        ScriptLibrary library = invoker.getLibrary();
         atts.put(Label.PROCEDURE_IMPLEMENTATION,
-                 PrimitiveExplainer.getInstance(invoker.getEngineName()));
+                 PrimitiveExplainer.getInstance(library.getEngineName()));
         atts.put(Label.PROCEDURE_IMPLEMENTATION, 
                  PrimitiveExplainer.getInstance(invoker.getFunctionName()));
-        if (invoker.isCompiled())
+        if (library.isCompiled())
             atts.put(Label.PROCEDURE_IMPLEMENTATION, 
                      PrimitiveExplainer.getInstance("compiled"));
         pool.put(invoker, true);        
 
=== modified file 'src/test/java/com/akiban/server/service/routines/MockRoutineLoader.java'
--- src/test/java/com/akiban/server/service/routines/MockRoutineLoader.java	2013-03-22 20:05:57 +0000
+++ src/test/java/com/akiban/server/service/routines/MockRoutineLoader.java	2013-04-11 23:35:24 +0000
@@ -62,6 +62,11 @@
     }
 
     @Override
+    public ScriptPool<ScriptLibrary> getScriptLibrary(Session session, TableName routineName) {
+        throw new UnsupportedOperationException();
+    }
+
+    @Override
     public void unloadRoutine(Session session, TableName routineName) {
         throw new UnsupportedOperationException();
     }