[GR-54136] Synchronize on thread registration when created from guest Thread.start().

PullRequest: graal/21885

Author: rakachan

espresso/src/com.oracle.truffle.espresso/src/com/oracle/truffle/espresso/runtime/EspressoContext.java

@@ -1104,8 +1104,9 @@ public StaticObject[] getActiveThreads() {
         return espressoEnv.getThreadRegistry().activeThreads();
     }
 
-    public void registerThread(Thread host, StaticObject self) {
-        espressoEnv.getThreadRegistry().registerThread(host, self);
+    public void registerJavaThread(Thread host, StaticObject self) {
+        StaticObject guest = espressoEnv.getThreadRegistry().registerThread(host, self);
+        assert self == guest;
         if (shouldReportVMEvents()) {
             espressoEnv.getEventListener().threadStarted(self);
         }

espresso/src/com.oracle.truffle.espresso/src/com/oracle/truffle/espresso/runtime/EspressoThreadLocalState.java

@@ -26,6 +26,7 @@
 
 import com.oracle.truffle.api.CompilerDirectives;
 import com.oracle.truffle.espresso.impl.ClassRegistry;
+import com.oracle.truffle.espresso.impl.Field;
 import com.oracle.truffle.espresso.runtime.staticobject.StaticObject;
 import com.oracle.truffle.espresso.vm.VM;
 
@@ -244,28 +245,30 @@ assert getHost(currentPlatformThread) == Thread.currentThread() : //
             assert getHost(t) == Thread.currentThread() : //
                     "Current thread fast access set by non-current thread";
 
+            EspressoContext ctx = EspressoContext.get(null);
+            Field managedBit = ctx.getMeta().HIDDEN_ESPRESSO_MANAGED;
+
             // Ensure we are not registering multiple guest threads for the same host thread.
             assert currentPlatformThread == null || currentPlatformThread == t : //
                     /*- Report these threads names */
                     getHost(currentPlatformThread).getName() + " vs " + getHost(t).getName() + "\n" +
                     /*- Report these threads identities */
-                    "Guest identities" + System.identityHashCode(currentPlatformThread) + " vs " + System.identityHashCode(t) + "\n" +
+                    "Guest identities: " + System.identityHashCode(currentPlatformThread) + " vs " + System.identityHashCode(t) + "\n" +
                     /*- Checks if our host threads are actually different, or if it is simply a renamed one. */
-                    "Host identities: " + System.identityHashCode(getHost(currentPlatformThread)) + " vs " + System.identityHashCode(getHost(t));
+                    "Host identities: " + System.identityHashCode(getHost(currentPlatformThread)) + " vs " + System.identityHashCode(getHost(t)) + "/n" +
+                    /*- Obtain the `managed` bits to know the origin of these threads */
+                    "Managed by espresso: " + managedBit.getBoolean(currentPlatformThread) + " vs " + managedBit.getBoolean(t);
         }
         // @formatter:on
-        /*-
+        /*
          * Current theory for GR-50089:
          *
-         * For some reason, when creating a new guest thread, instead of spawning a new host thread
-         * Truffle gives us back a previously created one that had completed.
-         *
-         * If that is the case, then, on failure, we will see in the report:
-         * - Different guest names and/or guest identities
-         * - Same host identities
+         * Lack of synchronization in `ThreadAccess.createJavaThread()` makes it so the polyglot
+         * thread is started and can reach `EspressoLanguage.initializeThread()` before the store to
+         * the thread registry can be observed.
          *
-         * This may be solved by unregistering a guest thread from the thread local state in
-         * ThreadAccess.terminate().
+         * Now that a `synchronize` block has been added to `EspressoThreadRegistry`, this assertion
+         * should no longer trigger.
          */
         return true;
     }

espresso/src/com.oracle.truffle.espresso/src/com/oracle/truffle/espresso/threads/EspressoThreadRegistry.java

@@ -117,45 +117,50 @@ private void registerMainThread(Thread thread, StaticObject guest) {
     public final AtomicLong createdThreadCount = new AtomicLong();
     public final AtomicLong peakThreadCount = new AtomicLong();
 
-    public void registerThread(Thread host, StaticObject guest) {
-        activeThreads.add(guest);
-
-        // Update java.lang.management counters.
-        createdThreadCount.incrementAndGet();
-        peakThreadCount.updateAndGet(new LongUnaryOperator() {
-            @Override
-            public long applyAsLong(long oldPeak) {
-                return Math.max(oldPeak, activeThreads.size());
+    public StaticObject registerThread(Thread host, StaticObject guest) {
+        synchronized (activeThreadLock) {
+            StaticObject existingThread = getGuestThreadFromHost(host);
+            if (existingThread != null) {
+                // There is already a guest thread for this host thread.
+                return existingThread;
             }
-        });
 
-        if (finalizerThreadId == -1) {
-            if (getMeta().java_lang_ref_Finalizer$FinalizerThread == guest.getKlass()) {
-                synchronized (activeThreadLock) {
+            activeThreads.add(guest);
+
+            // Update java.lang.management counters.
+            createdThreadCount.incrementAndGet();
+            peakThreadCount.updateAndGet(new LongUnaryOperator() {
+                @Override
+                public long applyAsLong(long oldPeak) {
+                    return Math.max(oldPeak, activeThreads.size());
+                }
+            });
+
+            if (finalizerThreadId == -1) {
+                if (getMeta().java_lang_ref_Finalizer$FinalizerThread == guest.getKlass()) {
                     if (finalizerThreadId == -1) {
                         CompilerDirectives.transferToInterpreterAndInvalidate();
                         finalizerThreadId = getThreadId(host);
                         guestFinalizerThread = guest;
-                        return;
+                        return guest;
                     }
                 }
             }
-        }
-        if (referenceHandlerThreadId == -1) {
-            if (getMeta().java_lang_ref_Reference$ReferenceHandler == guest.getKlass()) {
-                synchronized (activeThreadLock) {
+            if (referenceHandlerThreadId == -1) {
+                if (getMeta().java_lang_ref_Reference$ReferenceHandler == guest.getKlass()) {
                     if (referenceHandlerThreadId == -1) {
                         CompilerDirectives.transferToInterpreterAndInvalidate();
                         referenceHandlerThreadId = getThreadId(host);
                         guestReferenceHandlerThread = guest;
-                        return;
+                        return guest;
                     }
                 }
             }
-        }
-        pushThread(Math.toIntExact(getThreadId(host)), guest);
-        if (host == Thread.currentThread()) {
-            getContext().registerCurrentThread(guest);
+            pushThread(Math.toIntExact(getThreadId(host)), guest);
+            if (host == Thread.currentThread()) {
+                getContext().registerCurrentThread(guest);
+            }
+            return guest;
         }
     }
 
@@ -166,18 +171,18 @@ public long applyAsLong(long oldPeak) {
      */
     @SuppressFBWarnings(value = "NN", justification = "Removing a thread from the active set is the state change we need.")
     public boolean unregisterThread(StaticObject thread) {
-        if (!activeThreads.remove(thread)) {
-            // Already unregistered
-            return false;
-        }
-        logger.fine(() -> {
-            String guestName = getThreadAccess().getThreadName(thread);
-            long guestId = getThreadAccess().getThreadId(thread);
-            return String.format("unregisterThread([GUEST:%s, %d])", guestName, guestId);
-        });
-        Thread hostThread = getThreadAccess().getHost(thread);
-        int id = Math.toIntExact(getThreadId(hostThread));
         synchronized (activeThreadLock) {
+            if (!activeThreads.remove(thread)) {
+                // Already unregistered
+                return false;
+            }
+            logger.fine(() -> {
+                String guestName = getThreadAccess().getThreadName(thread);
+                long guestId = getThreadAccess().getThreadId(thread);
+                return String.format("unregisterThread([GUEST:%s, %d])", guestName, guestId);
+            });
+            Thread hostThread = getThreadAccess().getHost(thread);
+            int id = Math.toIntExact(getThreadId(hostThread));
             if (id == mainThreadId) {
                 mainThreadId = -1;
                 guestMainThread = null;
@@ -264,16 +269,17 @@ public StaticObject createGuestThreadFromHost(Thread hostThread, Meta meta, VM v
             return null;
         }
         synchronized (activeThreadLock) {
-            StaticObject exisitingThread = getGuestThreadFromHost(hostThread);
-            if (exisitingThread != null) {
+            StaticObject existingThread = getGuestThreadFromHost(hostThread);
+            if (existingThread != null) {
                 // already a live guest thread for this host thread
-                return exisitingThread;
+                return existingThread;
             }
             StaticObject effectiveThreadGroup = threadGroup;
             if (effectiveThreadGroup == null || StaticObject.isNull(effectiveThreadGroup)) {
                 effectiveThreadGroup = getContext().getMainThreadGroup();
             }
             vm.attachThread(hostThread);
+
             StaticObject guestThread = meta.java_lang_Thread.allocateInstance(getContext());
 
             // Allow guest Thread.currentThread() to work.
@@ -282,7 +288,9 @@ public StaticObject createGuestThreadFromHost(Thread hostThread, Meta meta, VM v
             }
             getThreadAccess().setEETopAlive(guestThread);
             getThreadAccess().initializeHiddenFields(guestThread, hostThread, managedByEspresso);
-            registerThread(hostThread, guestThread);
+            // Associate host and guest.
+            existingThread = registerThread(hostThread, guestThread);
+            assert existingThread == guestThread;
             assert getThreadAccess().getCurrentGuestThread() != null;
 
             if (name == null) {

espresso/src/com.oracle.truffle.espresso/src/com/oracle/truffle/espresso/threads/ThreadAccess.java

@@ -449,18 +449,20 @@ public boolean isManaged(StaticObject guest) {
      */
     public Thread createJavaThread(StaticObject guest, DirectCallNode exit, DirectCallNode dispatch) {
         Thread host = getContext().getEnv().newTruffleThreadBuilder(new GuestRunnable(getContext(), guest, exit, dispatch)).build();
-        initializeHiddenFields(guest, host, true);
         // Prepare host thread
         host.setDaemon(isDaemon(guest));
         host.setPriority(getPriority(guest));
+        String guestName = getContext().getThreadAccess().getThreadName(guest);
+        host.setName(guestName);
         if (isInterrupted(guest, false)) {
             host.interrupt();
         }
-        String guestName = getContext().getThreadAccess().getThreadName(guest);
-        host.setName(guestName);
+        // Prepare guest thread
+        initializeHiddenFields(guest, host, true);
         getThreadAccess().setEETopAlive(guest);
-        // Make the thread known to the context
-        getContext().registerThread(host, guest);
+        // Associate host and guest
+        getContext().registerJavaThread(host, guest);
+
         // Thread must be runnable on returning from 'start', so we set it preemptively
         // here.
         getThreadAccess().initializeState(guest, ThreadState.DefaultStates.DEFAULT_RUNNABLE_STATE);