fix: multithreadedJS should use concurrent java maps (#1920)

Author: edusperoni

test-app/app/src/main/assets/app/mainpage.js

@@ -72,5 +72,6 @@ require('./tests/testURLImpl.js');
 require('./tests/testURLSearchParamsImpl.js');
 require('./tests/testPerformanceNow');
 require('./tests/testQueueMicrotask');
+require("./tests/testConcurrentAccess");
 
 require("./tests/testESModules.mjs");

test-app/app/src/main/assets/app/tests/testConcurrentAccess.js

@@ -0,0 +1,78 @@
+// WARNING: IF THIS TEST FAILS IT COMPLETELY BREAKS ALL OTHER TESTS!
+
+describe("Tests concurrent access to JNI", function () {
+  // Customizable test parameters
+  const BACKGROUND_THREADS = 5;
+  const SYNC_CALLS = 2;
+  const ITERATIONS_PER_CALL = 100;
+  const TIMEOUT_MS = 3000;
+
+  it("test_high_contention_concurrent_access_with_multiple_objects", (done) => {
+    console.log('STARTING PROBLEMATIC TEST. THIS MIGHT CRASH OR CAUSE ISSUES IN OTHER TESTS IF IT FAILS. If this is close to the end of the log, check test_high_contention_concurrent_access_with_multiple_objects');
+    let callbackInvocations = 0;
+
+    const callback = new com.tns.tests.ConcurrentAccessTest.Callback({
+      invoke: (
+        list1,
+        list2,
+        list3,
+        list4,
+        list5,
+        list6,
+        list7,
+        list8,
+        list9,
+        list10,
+      ) => {
+        callbackInvocations++;
+        // Assert that accessing size() on any of the lists doesn't throw
+        expect(() => list1.size()).not.toThrow();
+        expect(() => list2.size()).not.toThrow();
+        expect(() => list3.size()).not.toThrow();
+        expect(() => list4.size()).not.toThrow();
+        expect(() => list5.size()).not.toThrow();
+        expect(() => list6.size()).not.toThrow();
+        expect(() => list7.size()).not.toThrow();
+        expect(() => list8.size()).not.toThrow();
+        expect(() => list9.size()).not.toThrow();
+        expect(() => list10.size()).not.toThrow();
+
+        // Verify that the lists actually have content
+        expect(list1.size()).toBe(5);
+        expect(list2.size()).toBe(5);
+        expect(list3.size()).toBe(5);
+        expect(list4.size()).toBe(5);
+        expect(list5.size()).toBe(5);
+        expect(list6.size()).toBe(5);
+        expect(list7.size()).toBe(5);
+        expect(list8.size()).toBe(5);
+        expect(list9.size()).toBe(5);
+        expect(list10.size()).toBe(5);
+      },
+    });
+
+    // Start multiple background threads
+    for (let i = 0; i < BACKGROUND_THREADS; i++) {
+      com.tns.tests.ConcurrentAccessTest.callFromBackgroundThread(
+        callback,
+        ITERATIONS_PER_CALL,
+      );
+    }
+
+    // Call synchronously multiple times
+    for (let i = 0; i < SYNC_CALLS; i++) {
+      com.tns.tests.ConcurrentAccessTest.callSynchronously(
+        callback,
+        ITERATIONS_PER_CALL,
+      );
+    }
+
+    // Wait for all threads to complete
+    setTimeout(() => {
+      const expectedInvocations =
+        (BACKGROUND_THREADS + SYNC_CALLS) * ITERATIONS_PER_CALL;
+      expect(callbackInvocations).toBe(expectedInvocations);
+      done();
+    }, TIMEOUT_MS);
+  });
+});

test-app/app/src/main/java/com/tns/tests/ConcurrentAccessTest.java

@@ -0,0 +1,76 @@
+package com.tns.tests;
+
+import java.util.ArrayList;
+
+public class ConcurrentAccessTest {
+    
+    public interface Callback {
+        void invoke(ArrayList list1, ArrayList list2, ArrayList list3, ArrayList list4, ArrayList list5,
+                   ArrayList list6, ArrayList list7, ArrayList list8, ArrayList list9, ArrayList list10);
+    }
+    
+    public interface ErrorCallback {
+        void onError(Throwable error);
+    }
+    
+    /**
+     * Calls the callback from a background thread multiple times.
+     * @param callback The callback to invoke
+     * @param times Number of times to call the callback (default 50)
+     */
+    public static void callFromBackgroundThread(final Callback callback, final int times) {
+        Thread thread = new Thread(new Runnable() {
+            @Override
+            public void run() {
+                for (int i = 0; i < times; i++) {
+                    invokeCallbackWithArrayLists(callback, i);
+                }
+            }
+        });
+        thread.start();
+    }
+    
+    /**
+     * Calls the callback synchronously from the current thread.
+     * @param callback The callback to invoke
+     * @param times Number of times to call the callback (default 50)
+     */
+    public static void callSynchronously(Callback callback, int times) {
+        for (int i = 0; i < times; i++) {
+            invokeCallbackWithArrayLists(callback, i);
+        }
+    }
+    
+    /**
+     * Helper method that creates 10 ArrayLists and invokes the callback with them.
+     * Each ArrayList contains some data based on the iteration number.
+     */
+    private static void invokeCallbackWithArrayLists(Callback callback, int iteration) {
+        ArrayList<Integer> list1 = new ArrayList<>();
+        ArrayList<Integer> list2 = new ArrayList<>();
+        ArrayList<Integer> list3 = new ArrayList<>();
+        ArrayList<Integer> list4 = new ArrayList<>();
+        ArrayList<Integer> list5 = new ArrayList<>();
+        ArrayList<Integer> list6 = new ArrayList<>();
+        ArrayList<Integer> list7 = new ArrayList<>();
+        ArrayList<Integer> list8 = new ArrayList<>();
+        ArrayList<Integer> list9 = new ArrayList<>();
+        ArrayList<Integer> list10 = new ArrayList<>();
+        
+        // Add some data to each list
+        for (int i = 0; i < 5; i++) {
+            list1.add(iteration * 10 + i);
+            list2.add(iteration * 10 + i + 1);
+            list3.add(iteration * 10 + i + 2);
+            list4.add(iteration * 10 + i + 3);
+            list5.add(iteration * 10 + i + 4);
+            list6.add(iteration * 10 + i + 5);
+            list7.add(iteration * 10 + i + 6);
+            list8.add(iteration * 10 + i + 7);
+            list9.add(iteration * 10 + i + 8);
+            list10.add(iteration * 10 + i + 9);
+        }
+        
+        callback.invoke(list1, list2, list3, list4, list5, list6, list7, list8, list9, list10);
+    }
+}

test-app/runtime/src/main/java/com/tns/Runtime.java

@@ -35,6 +35,7 @@
 import java.util.concurrent.FutureTask;
 import java.util.concurrent.RunnableFuture;
 import java.util.concurrent.atomic.AtomicInteger;
+import java.util.Collections;
 
 public class Runtime {
     private native void initNativeScript(int runtimeId, String filesPath, String nativeLibDir, boolean verboseLoggingEnabled, boolean isDebuggable, String packageName,
@@ -103,15 +104,15 @@ public static void passSuppressedExceptionToJs(Throwable ex, String methodName)
             "Primitive types need to be manually wrapped in their respective Object wrappers.\n" +
             "If you are creating an instance of an inner class, make sure to always provide reference to the outer `this` as the first argument.";
 
-    private HashMap<Integer, Object> strongInstances = new HashMap<>();
+    private Map<Integer, Object> strongInstances = new HashMap<>();
 
-    private HashMap<Integer, WeakReference<Object>> weakInstances = new HashMap<>();
+    private Map<Integer, WeakReference<Object>> weakInstances = new HashMap<>();
 
-    private NativeScriptHashMap<Object, Integer> strongJavaObjectToID = new NativeScriptHashMap<Object, Integer>();
+    private Map<Object, Integer> strongJavaObjectToID = new NativeScriptHashMap<Object, Integer>();
 
-    private NativeScriptWeakHashMap<Object, Integer> weakJavaObjectToID = new NativeScriptWeakHashMap<Object, Integer>();
+    private Map<Object, Integer> weakJavaObjectToID = new NativeScriptWeakHashMap<Object, Integer>();
 
-    private final Map<Class<?>, JavaScriptImplementation> loadedJavaScriptExtends = new HashMap<Class<?>, JavaScriptImplementation>();
+    private Map<Class<?>, JavaScriptImplementation> loadedJavaScriptExtends = new HashMap<Class<?>, JavaScriptImplementation>();
 
     private final java.lang.Runtime dalvikRuntime = java.lang.Runtime.getRuntime();
 
@@ -215,6 +216,16 @@ public Runtime(StaticConfiguration config, DynamicConfiguration dynamicConfigura
                 if (dynamicConfiguration.mainThreadScheduler != null) {
                     this.mainThreadHandler = dynamicConfiguration.mainThreadScheduler.getHandler();
                 }
+                // if multithreadedJS, make all maps concurrent or synchronized:
+                if (config.appConfig.getEnableMultithreadedJavascript()) {
+                    this.strongInstances = new ConcurrentHashMap<>();
+                    this.weakInstances = new ConcurrentHashMap<>();
+                    // TODO: can't use a ConcurrentHashMap for loadedJavaScriptExtends because it loads null objects, which aren't supported
+                    // either leave it like this or create a separate set for null caches
+                    this.loadedJavaScriptExtends = Collections.synchronizedMap(new HashMap<>());
+                    this.strongJavaObjectToID = Collections.synchronizedMap(new NativeScriptHashMap<>());
+                    this.weakJavaObjectToID = Collections.synchronizedMap(new NativeScriptWeakHashMap<>());
+                }
 
                 classResolver = new ClassResolver(classStorageService);
                 currentRuntime.set(this);