[GR-59795] Random failures in JS Test262.

PullRequest: graal/19309

Author: tzezula

sdk/src/org.graalvm.polyglot/src/org/graalvm/polyglot/Engine.java

@@ -946,6 +946,11 @@ public Engine newEngine(AbstractEngineDispatch dispatch, Object receiver, boolea
             return engine;
         }
 
+        @Override
+        public void processReferenceQueue() {
+            CleanableReference.processReferenceQueue();
+        }
+
         @Override
         public void engineClosed(Reference<Engine> engineReference) {
             ENGINES.remove(engineReference);
@@ -2056,7 +2061,6 @@ private abstract static class CleanableReference<T> extends WeakReference<T> {
 
         protected CleanableReference(T referent) {
             super(referent, QUEUE);
-            processReferenceQueue();
         }
 
         protected abstract void clean();

sdk/src/org.graalvm.polyglot/src/org/graalvm/polyglot/impl/AbstractPolyglotImpl.java

@@ -160,6 +160,8 @@ protected APIAccess() {
 
         public abstract Context newInnerContext(AbstractContextDispatch dispatch, Object receiver, Context parentContext, Engine engine);
 
+        public abstract void processReferenceQueue();
+
         public abstract Language newLanguage(AbstractLanguageDispatch dispatch, Object receiver, Engine engine);
 
         public abstract Instrument newInstrument(AbstractInstrumentDispatch dispatch, Object receiver, Engine engine);

truffle/src/com.oracle.truffle.api.test/src/com/oracle/truffle/api/test/polyglot/PolyglotGCTest.java

@@ -89,12 +89,16 @@
 import java.util.Collection;
 import java.util.List;
 import java.util.Map;
+import java.util.concurrent.CompletionService;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorCompletionService;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.Future;
+import java.util.concurrent.Semaphore;
+import java.util.concurrent.ThreadPoolExecutor;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
 import java.util.concurrent.atomic.AtomicBoolean;
@@ -864,6 +868,81 @@ public void run() {
         }
     }
 
+    @Test
+    public void testGR59795() throws Exception {
+        TruffleTestAssumptions.assumeWeakEncapsulation();
+        runInSubprocess(() -> {
+            List<Context> contexts = new ArrayList<>(100);
+            // Create top-level contexts
+            for (int i = 0; i < 100; i++) {
+                contexts.add(newContextBuilder().build());
+            }
+
+            // In each top-level context create a strongly reachable inner context
+            for (Context context : contexts) {
+                AbstractExecutableTestLanguage.execute(context, TestGR59795Language.class);
+            }
+
+            // Make the inner context unreachable and assert that are collected
+            List<? extends Reference<TruffleContext>> innerContextRefs = TestGR59795Language.clearInnerContexts();
+            List<Pair<String, Reference<?>>> msgRefPairs = new ArrayList<>(innerContextRefs.size());
+            for (Reference<?> reference : innerContextRefs) {
+                msgRefPairs.add(Pair.create("Inner context must be collected", reference));
+            }
+            GCUtils.assertGc(msgRefPairs);
+
+            // Dispose top-level contexts in parallel
+            ThreadPoolExecutor executor = (ThreadPoolExecutor) Executors.newFixedThreadPool(20);
+            CompletionService<Boolean> todo = new ExecutorCompletionService<>(executor);
+            Semaphore threadsStarted = new Semaphore(-1 * executor.getMaximumPoolSize());
+            CountDownLatch runClose = new CountDownLatch(1);
+            for (Context context : contexts) {
+                todo.submit(() -> {
+                    threadsStarted.release();
+                    runClose.await();
+                    context.close();
+                    return true;
+                });
+            }
+            threadsStarted.release();
+            try {
+                threadsStarted.acquire();
+            } catch (InterruptedException ie) {
+                throw new AssertionError(ie);
+            }
+            runClose.countDown();
+            for (int i = 0; i < contexts.size(); i++) {
+                try {
+                    todo.take();
+                } catch (InterruptedException e) {
+                    throw new AssertionError(e);
+                }
+            }
+            executor.shutdown();
+        });
+    }
+
+    @Registration
+    static final class TestGR59795Language extends AbstractPolyglotGcLanguage {
+
+        private static final List<TruffleContext> innerContexts = new ArrayList<>();
+
+        static List<? extends Reference<TruffleContext>> clearInnerContexts() {
+            List<? extends Reference<TruffleContext>> res = TestGR59795Language.innerContexts.stream().map(WeakReference::new).toList();
+            TestGR59795Language.innerContexts.clear();
+            return res;
+        }
+
+        @Override
+        @TruffleBoundary
+        protected Object execute(RootNode node, Env env, Object[] contextArguments, Object[] frameArguments) throws Exception {
+            TruffleContext innerContext = env.newInnerContextBuilder().build();
+            innerContext.initializePublic(node, TestUtils.getDefaultLanguageId(TestGR59795Language.class));
+            innerContexts.add(innerContext);
+            return true;
+        }
+    }
+
     @SuppressWarnings("try")
     private static void assertContextGc(String message, Reference<Context> contextRef) {
         GCUtils.assertGc(message, contextRef);

truffle/src/com.oracle.truffle.api.test/src/com/oracle/truffle/api/test/wrapper/HostEngineDispatch.java

@@ -92,7 +92,11 @@ public Context createContext(Object receiver, Engine engineApi, SandboxPolicy sa
         long guestContextId = hostToGuest.remoteCreateContext(engine.remoteEngine, sandboxPolicy, tmpDir);
         HostContext context = new HostContext(engine, guestContextId, localContext);
         hostToGuest.registerHostContext(guestContextId, context);
-        return polyglot.getAPIAccess().newContext(remoteContext, context, engineApi, registerInActiveContexts);
+        Context contextApi = polyglot.getAPIAccess().newContext(remoteContext, context, engineApi, registerInActiveContexts);
+        if (registerInActiveContexts) {
+            polyglot.getAPIAccess().processReferenceQueue();
+        }
+        return contextApi;
     }
 
     @Override

truffle/src/com.oracle.truffle.api.test/src/com/oracle/truffle/api/test/wrapper/HostPolyglotDispatch.java

@@ -77,7 +77,11 @@ public Engine buildEngine(String[] permittedLanguages, SandboxPolicy sandboxPoli
                             onlyHostLanguage, false, polyglotHostService);
             long remoteEngine = getHostToGuest().remoteCreateEngine(sandboxPolicy);
             HostEngine engine = new HostEngine(remoteEngine, localEngine);
-            return getAPIAccess().newEngine(new HostEngineDispatch(this), engine, registerInActiveEngines);
+            Engine engineApi = getAPIAccess().newEngine(new HostEngineDispatch(this), engine, registerInActiveEngines);
+            if (registerInActiveEngines) {
+                getAPIAccess().processReferenceQueue();
+            }
+            return engineApi;
         } else {
             return getNext().buildEngine(permittedLanguages, sandboxPolicy, out, err, in, options, allowExperimentalOptions, boundEngine, messageInterceptor, logHandler,
                             hostLanguage,

truffle/src/com.oracle.truffle.polyglot/src/com/oracle/truffle/polyglot/EngineAccessor.java

@@ -1126,6 +1126,8 @@ public TruffleContext createInternalContext(Object sourcePolyglotLanguageContext
                 impl.setCreatorTruffleContextReference(new TruffleContextCleanableReference(creatorContext, impl));
                 creator.context.addChildContext(impl);
             }
+            creator.getImpl().getAPIAccess().processReferenceQueue();
+            TruffleContextCleanableReference.processReferenceQueue();
 
             synchronized (impl) {
                 impl.initializeContextLocals();

truffle/src/com.oracle.truffle.polyglot/src/com/oracle/truffle/polyglot/PolyglotContextImpl.java

@@ -614,7 +614,10 @@ Context getContextAPI() {
             } else {
                 /*
                  * For an inner context, it may be necessary to recreate the embedder API object
-                 * when it is reintroduced from an existing TruffleContext doing host call.
+                 * when it is reintroduced from an existing TruffleContext doing host call. Avoid
+                 * processing the reference queue here, as the current thread may be holding locks.
+                 * The context's references are handled when the context or engine is created or
+                 * closed.
                  */
                 result = getAPIAccess().newInnerContext(getImpl().contextDispatch, this, parent.getContextAPI(), engine.getEngineAPI());
             }
@@ -645,6 +648,11 @@ TruffleContext getCreatorTruffleContext() {
                      * object for instrument notification during the close.
                      */
                     truffleContext = EngineAccessor.LANGUAGE.createTruffleContext(this, parent.getCreatorTruffleContext());
+                    /*
+                     * Avoid processing the reference queue here, as the current thread may be
+                     * holding locks. The context's references are handled when the context or
+                     * engine is created or closed.
+                     */
                     weakCreatorTruffleContext = new TruffleContextCleanableReference(truffleContext, this);
                 }
             }
@@ -3082,9 +3090,13 @@ private boolean finishClose(boolean cancelOrExitOperation, boolean notifyInstrum
          * still be required to create a resulting Value, PolyglotException or ResourceLimitEvent.
          * Additionally, an active cancelled context may have submitted a thread-local action to
          * throw a CancelExecution exception. Closing a collected context at this place would
-         * execute this thread-local action, causing the exception to be lost.
+         * execute this thread-local action, causing the exception to be lost. Also avoid processing
+         * the reference queue while closing a child context as part of the parent context's
+         * closure. At this point, the parent's closeLock is still held. The reference queue will be
+         * processed later when the parent context itself is closed and the parent context's
+         * closeLock is no longer held.
          */
-        if (!active) {
+        if (!active && (parent == null || parent.closingThread != Thread.currentThread())) {
             CompilerAsserts.neverPartOfCompilation("Direct access to weakCreatorTruffleContext");
             getAPIAccess().contextClosed(weakAPI);
             if (parent == null) {
@@ -3469,8 +3481,15 @@ private void finalizeContext(boolean notifyInstruments, boolean mustSucceed) {
             unclosedChildContexts = getUnclosedChildContexts();
         }
         for (PolyglotContextImpl childCtx : unclosedChildContexts) {
-            if (childCtx.isActive()) {
-                throw new IllegalStateException("There is an active child contexts after finalizeContext!");
+            /*
+             * Another thread might be closing an unreachable child context. Closing a context
+             * requires it to be entered and made active. This situation is acceptable, as
+             * closeChildContexts will wait for the other thread to finish closing the context.
+             */
+            synchronized (childCtx) {
+                if (childCtx.isActive() && !childCtx.state.isClosing()) {
+                    throw new IllegalStateException("There is an active child context after finalizeContext!");
+                }
             }
         }
         if (!unclosedChildContexts.isEmpty()) {

truffle/src/com.oracle.truffle.polyglot/src/com/oracle/truffle/polyglot/PolyglotEngineImpl.java

@@ -1931,6 +1931,14 @@ public Context createContext(Engine engineAPI, SandboxPolicy contextSandboxPolic
                 }
             }
         }
+        /*
+         * When registerInActiveContext is false, the context is a local context decorated by its
+         * owning context. In this case, there's no need to process the reference queue, as it will
+         * be processed by the owner.
+         */
+        if (registerInActiveContexts) {
+            getAPIAccess().processReferenceQueue();
+        }
         return contextAPI;
     }
 

truffle/src/com.oracle.truffle.polyglot/src/com/oracle/truffle/polyglot/PolyglotImpl.java

@@ -352,7 +352,16 @@ public Engine buildEngine(String[] permittedLanguages, SandboxPolicy sandboxPoli
                                 hostLanguageOnly,
                                 usePolyglotHostService);
             }
-            return getAPIAccess().newEngine(engineDispatch, impl, registerInActiveEngines);
+            Engine engineApi = getAPIAccess().newEngine(engineDispatch, impl, registerInActiveEngines);
+            /*
+             * When registerInActiveEngines is false, the engine is a local engine decorated by its
+             * owning engine. In this case, there's no need to process the reference queue, as it
+             * will be processed by the owner.
+             */
+            if (registerInActiveEngines) {
+                getAPIAccess().processReferenceQueue();
+            }
+            return engineApi;
         } catch (Throwable t) {
             if (impl == null) {
                 throw PolyglotImpl.guestToHostException(this, t);

truffle/src/com.oracle.truffle.polyglot/src/com/oracle/truffle/polyglot/TruffleContextCleanableReference.java

@@ -56,7 +56,6 @@ final class TruffleContextCleanableReference extends WeakReference<TruffleContex
     TruffleContextCleanableReference(TruffleContext referent, PolyglotContextImpl polyglotContext) {
         super(referent, queue);
         this.polyglotContext = Objects.requireNonNull(polyglotContext, "PolyglotContext must be non null");
-        processReferenceQueue();
     }
 
     static void processReferenceQueue() {