Fixed https://github.com/couchbase/couchbase-lite-java-core/issues/1375

hideki · hideki · commit a83ed6ae626e · 2016-07-27T17:18:07.000-07:00
Issue: Reduce intermittently returns wrong results for 1.3.0-45 As posted in https://github.com/couchbase/couchbase-lite-java-core/issues/1375#issuecomment-235748848, given keys and values are correct, but result from reduce function is incorrect. According to Rhino documentation (https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Rhino/Scopes_and_Contexts), Scope is sharable from multiple threads, but others are not. CBL Java/Android used to compile given JavaScript for every requests. But it makes JavaScript map/reduce function makes really slow. So we switched to "compile once, and reuse it" style. It seems accessing `Function` instance from multiple threads is not safe. So, by this fix, synchronized execution block.
diff --git a/src/main/java/com/couchbase/lite/javascript/ReplicationFilterBlockRhino.java b/src/main/java/com/couchbase/lite/javascript/ReplicationFilterBlockRhino.java
@@ -1,17 +1,16 @@
-/**
- * Copyright (c) 2015 Couchbase, Inc All rights reserved.
- *
- * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file
- * except in compliance with the License. You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software distributed under the
- * License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND,
- * either express or implied. See the License for the specific language governing permissions
- * and limitations under the License.
- */
-
+//
+// Copyright (c) 2016 Couchbase, Inc. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file
+// except in compliance with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software distributed under the
+// License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND,
+// either express or implied. See the License for the specific language governing permissions
+// and limitations under the License.
+//
 package com.couchbase.lite.javascript;
 
 import com.couchbase.lite.ReplicationFilter;
@@ -30,14 +29,21 @@
  * Created by hideki on 10/28/15.
  */
 public class ReplicationFilterBlockRhino implements ReplicationFilter {
-    public static String TAG = "ReplicationFilterBlockRhino";
+    public static String TAG = "JavaScriptEngine";
 
     private static WrapFactory wrapFactory = new CustomWrapFactory();
     private Scriptable scope;
     private GlobalScope globalScope;
     private Function filterFunction;
 
-    public ReplicationFilterBlockRhino(String src){
+    // NOTE: Scope is sharable with multiple threads, it seems `Function` is not.
+    //       Compiling javascript codes for every request makes slow.
+    //       It is reason current code base re-use compiled Function.
+    //       Instead of compiling for every request, use `synchronized` to protect Function
+    //       https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Rhino/Scopes_and_Contexts
+    private final Object lockFunction = new Object();
+
+    public ReplicationFilterBlockRhino(String src) {
         org.mozilla.javascript.Context ctx = org.mozilla.javascript.Context.enter();
         try {
             ctx.setOptimizationLevel(-1);
@@ -52,28 +58,27 @@ public ReplicationFilterBlockRhino(String src){
 
     @Override
     public boolean filter(SavedRevision revision, Map<String, Object> params) {
-        org.mozilla.javascript.Context ctx = org.mozilla.javascript.Context.enter();
-        try {
-            ctx.setOptimizationLevel(-1);
-            ctx.setWrapFactory(wrapFactory);
-
-            Scriptable localScope = ctx.newObject(scope);
-            localScope.setPrototype(scope);
-            localScope.setParentScope(null);
-
-            Object jsDocument = org.mozilla.javascript.Context.javaToJS(revision.getProperties(), localScope);
-            Object jsParams = org.mozilla.javascript.Context.javaToJS(params, localScope);
-
+        synchronized (lockFunction) {
+            org.mozilla.javascript.Context ctx = org.mozilla.javascript.Context.enter();
             try {
-                Object result = filterFunction.call(ctx, localScope, null, new Object[]{jsDocument, jsParams});
-                return ((Boolean)result).booleanValue();
-            } catch (org.mozilla.javascript.RhinoException e) {
-                // Error in the JavaScript view - CouchDB swallows  the error and tries the next document
-                Log.e(TAG, "Error in filterFunction.call()", e);
-                return false;
+                ctx.setOptimizationLevel(-1);
+                ctx.setWrapFactory(wrapFactory);
+                Scriptable localScope = ctx.newObject(scope);
+                localScope.setPrototype(scope);
+                localScope.setParentScope(null);
+                Object jsDocument = org.mozilla.javascript.Context.javaToJS(revision.getProperties(), localScope);
+                Object jsParams = org.mozilla.javascript.Context.javaToJS(params, localScope);
+
+                try {
+                    Object result = filterFunction.call(ctx, localScope, null, new Object[]{jsDocument, jsParams});
+                    return ((Boolean) result).booleanValue();
+                } catch (org.mozilla.javascript.RhinoException e) {
+                    Log.e(TAG, "Error in filterFunction.call()", e);
+                    return false;
+                }
+            } finally {
+                org.mozilla.javascript.Context.exit();
             }
-        } finally {
-            org.mozilla.javascript.Context.exit();
         }
     }
 }
diff --git a/src/main/java/com/couchbase/lite/javascript/ViewMapBlockRhino.java b/src/main/java/com/couchbase/lite/javascript/ViewMapBlockRhino.java
@@ -1,22 +1,23 @@
-/**
- * Copyright (c) 2015 Couchbase, Inc All rights reserved.
- *
- * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file
- * except in compliance with the License. You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software distributed under the
- * License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND,
- * either express or implied. See the License for the specific language governing permissions
- * and limitations under the License.
- */
+//
+// Copyright (c) 2016 Couchbase, Inc. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file
+// except in compliance with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software distributed under the
+// License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND,
+// either express or implied. See the License for the specific language governing permissions
+// and limitations under the License.
+//
 package com.couchbase.lite.javascript;
 
 import com.couchbase.lite.Emitter;
 import com.couchbase.lite.Mapper;
 import com.couchbase.lite.javascript.scopes.MapGlobalScope;
 import com.couchbase.lite.javascript.wrapper.CustomWrapFactory;
+import com.couchbase.lite.util.Log;
 
 import org.mozilla.javascript.Function;
 import org.mozilla.javascript.Scriptable;
@@ -25,16 +26,22 @@
 import java.util.Map;
 
 class ViewMapBlockRhino implements Mapper {
+    private final static String TAG = "JavaScriptEngine";
 
     private static WrapFactory wrapFactory = new CustomWrapFactory();
     private Scriptable globalScope;
     private MapGlobalScope mapGlobalScope;
     private Function mapFunction;
 
-    public ViewMapBlockRhino(String src) {
+    // NOTE: Scope is sharable with multiple threads, it seems `Function` is not.
+    //       Compiling javascript codes for every request makes slow.
+    //       It is reason current code base re-use compiled Function.
+    //       Instead of compiling for every request, use `synchronized` to protect Function
+    //       https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Rhino/Scopes_and_Contexts
+    private final Object lockFunction = new Object();
 
+    public ViewMapBlockRhino(String src) {
         org.mozilla.javascript.Context ctx = org.mozilla.javascript.Context.enter();
-
         try {
             ctx.setOptimizationLevel(-1);
             ctx.setWrapFactory(wrapFactory);
@@ -48,28 +55,29 @@ public ViewMapBlockRhino(String src) {
 
     @Override
     public void map(Map<String, Object> document, Emitter emitter) {
+        synchronized (lockFunction) {
+            mapGlobalScope.setEmitter(emitter);
 
-        mapGlobalScope.setEmitter(emitter);
-
-        org.mozilla.javascript.Context ctx = org.mozilla.javascript.Context.enter();
-        try {
-            ctx.setOptimizationLevel(-1);
-            ctx.setWrapFactory(wrapFactory);
+            org.mozilla.javascript.Context ctx = org.mozilla.javascript.Context.enter();
+            try {
+                ctx.setOptimizationLevel(-1);
+                ctx.setWrapFactory(wrapFactory);
 
-            Scriptable localScope = ctx.newObject(globalScope);
-            localScope.setPrototype(globalScope);
-            localScope.setParentScope(null);
+                Scriptable localScope = ctx.newObject(globalScope);
+                localScope.setPrototype(globalScope);
+                localScope.setParentScope(null);
 
-            Object jsDocument = org.mozilla.javascript.Context.javaToJS(document, localScope);
+                Object jsDocument = org.mozilla.javascript.Context.javaToJS(document, localScope);
 
-            try {
-                mapFunction.call(ctx, localScope, null, new Object[]{jsDocument});
-            } catch (org.mozilla.javascript.RhinoException e) {
-                // Error in the JavaScript view - CouchDB swallows  the error and tries the next document
-                return;
+                try {
+                    mapFunction.call(ctx, localScope, null, new Object[]{jsDocument});
+                } catch (org.mozilla.javascript.RhinoException e) {
+                    Log.e(TAG, "Error in calling JavaScript map function", e);
+                    return;
+                }
+            } finally {
+                org.mozilla.javascript.Context.exit();
             }
-        } finally {
-            org.mozilla.javascript.Context.exit();
         }
     }
 }
diff --git a/src/main/java/com/couchbase/lite/javascript/ViewReduceBlockRhino.java b/src/main/java/com/couchbase/lite/javascript/ViewReduceBlockRhino.java
@@ -1,21 +1,22 @@
-/**
- * Copyright (c) 2015 Couchbase, Inc All rights reserved.
- *
- * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file
- * except in compliance with the License. You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software distributed under the
- * License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND,
- * either express or implied. See the License for the specific language governing permissions
- * and limitations under the License.
- */
+//
+// Copyright (c) 2016 Couchbase, Inc. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file
+// except in compliance with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software distributed under the
+// License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND,
+// either express or implied. See the License for the specific language governing permissions
+// and limitations under the License.
+//
 package com.couchbase.lite.javascript;
 
 import com.couchbase.lite.Reducer;
 import com.couchbase.lite.javascript.scopes.ReduceGlobalScope;
 import com.couchbase.lite.javascript.wrapper.CustomWrapFactory;
+import com.couchbase.lite.util.Log;
 
 import org.mozilla.javascript.Function;
 import org.mozilla.javascript.Scriptable;
@@ -25,20 +26,27 @@
 import java.util.List;
 
 class ViewReduceBlockRhino implements Reducer {
+    private final static String TAG = "JavaScriptEngine";
 
     private static WrapFactory wrapFactory = new CustomWrapFactory();
     private final ReduceGlobalScope reduceGlobalScope;
     private final ScriptableObject globalScope;
     private final Function reduceFunction;
     private final NativReduceFunctions nativeReduce;
 
+    // NOTE: Scope is sharable with multiple threads, it seems `Function` is not.
+    //       Compiling javascript codes for every request makes slow.
+    //       It is reason current code base re-use compiled Function.
+    //       Instead of compiling for every request, use `synchronized` to protect Function
+    //       https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Rhino/Scopes_and_Contexts
+    private final Object lockFunction = new Object();
+
     public ViewReduceBlockRhino(String src) {
 
         nativeReduce = NativReduceFunctions.fromKey(src);
 
         if (nativeReduce == NativReduceFunctions.DEFAULT) {
             org.mozilla.javascript.Context ctx = org.mozilla.javascript.Context.enter();
-
             try {
                 ctx.setOptimizationLevel(-1);
                 ctx.setWrapFactory(wrapFactory);
@@ -58,36 +66,37 @@ public ViewReduceBlockRhino(String src) {
 
     @Override
     public Object reduce(List<Object> keys, List<Object> values, boolean reReduce) {
-
         switch (nativeReduce) {
             case SUM:
                 return nativeSum(keys, values, reReduce);
             case COUNT:
                 return nativeCount(keys, values, reReduce);
             case DEFAULT:
             default:
-                org.mozilla.javascript.Context ctx = org.mozilla.javascript.Context.enter();
-                try {
-                    ctx.setOptimizationLevel(-1);
-                    ctx.setWrapFactory(wrapFactory);
-
-                    Scriptable localScope = ctx.newObject(globalScope);
-                    localScope.setPrototype(globalScope);
-                    localScope.setParentScope(null);
-
-                    Object[] args = new Object[3];
-
-                    args[0] = org.mozilla.javascript.Context.javaToJS(keys, localScope);
-                    args[1] = org.mozilla.javascript.Context.javaToJS(values, localScope);
-                    args[2] = org.mozilla.javascript.Context.javaToJS(reReduce, localScope);
-
-                    return reduceFunction.call(ctx, localScope, null, args);
-
-                } catch (org.mozilla.javascript.RhinoException e) {
-                    // TODO check couchdb behaviour on error in reduce function
-                    return null;
-                } finally {
-                    org.mozilla.javascript.Context.exit();
+                synchronized (lockFunction) {
+                    org.mozilla.javascript.Context ctx = org.mozilla.javascript.Context.enter();
+                    try {
+                        ctx.setOptimizationLevel(-1);
+                        ctx.setWrapFactory(wrapFactory);
+
+                        Scriptable localScope = ctx.newObject(globalScope);
+                        localScope.setPrototype(globalScope);
+                        localScope.setParentScope(null);
+
+                        Object[] args = new Object[3];
+                        args[0] = org.mozilla.javascript.Context.javaToJS(keys, localScope);
+                        args[1] = org.mozilla.javascript.Context.javaToJS(values, localScope);
+                        args[2] = org.mozilla.javascript.Context.javaToJS(reReduce, localScope);
+
+                        try {
+                            return reduceFunction.call(ctx, localScope, null, args);
+                        } catch (org.mozilla.javascript.RhinoException e) {
+                            Log.e(TAG, "Error in calling JavaScript reduce function", e);
+                            return null;
+                        }
+                    } finally {
+                        org.mozilla.javascript.Context.exit();
+                    }
                 }
         }
     }
