[GR-51440] Fix a Windows-specific thread handle leak.

PullRequest: graal/16637

substratevm/src/com.oracle.svm.core.posix/src/com/oracle/svm/core/posix/thread/PosixPlatformThreads.java

@@ -24,20 +24,14 @@
  */
 package com.oracle.svm.core.posix.thread;
 
-import jdk.graal.compiler.core.common.SuppressFBWarnings;
 import org.graalvm.nativeimage.ImageSingletons;
-import org.graalvm.nativeimage.ObjectHandle;
 import org.graalvm.nativeimage.Platform;
 import org.graalvm.nativeimage.Platform.HOSTED_ONLY;
 import org.graalvm.nativeimage.Platforms;
 import org.graalvm.nativeimage.StackValue;
 import org.graalvm.nativeimage.UnmanagedMemory;
-import org.graalvm.nativeimage.c.function.CEntryPoint;
-import org.graalvm.nativeimage.c.function.CEntryPoint.Publish;
-import org.graalvm.nativeimage.c.function.CEntryPointLiteral;
 import org.graalvm.nativeimage.c.function.CFunctionPointer;
 import org.graalvm.nativeimage.c.struct.SizeOf;
-import org.graalvm.nativeimage.c.type.CCharPointer;
 import org.graalvm.nativeimage.c.type.CTypeConversion;
 import org.graalvm.nativeimage.c.type.CTypeConversion.CCharPointerHolder;
 import org.graalvm.nativeimage.c.type.VoidPointer;
@@ -53,12 +47,6 @@
 import com.oracle.svm.core.annotate.Inject;
 import com.oracle.svm.core.annotate.RecomputeFieldValue;
 import com.oracle.svm.core.annotate.TargetClass;
-import com.oracle.svm.core.c.CGlobalData;
-import com.oracle.svm.core.c.CGlobalDataFactory;
-import com.oracle.svm.core.c.function.CEntryPointActions;
-import com.oracle.svm.core.c.function.CEntryPointErrors;
-import com.oracle.svm.core.c.function.CEntryPointOptions;
-import com.oracle.svm.core.c.function.CEntryPointSetup.LeaveDetachThreadEpilogue;
 import com.oracle.svm.core.feature.AutomaticallyRegisteredImageSingleton;
 import com.oracle.svm.core.graal.stackvalue.UnsafeStackValue;
 import com.oracle.svm.core.posix.PosixUtils;
@@ -77,9 +65,11 @@
 import com.oracle.svm.core.thread.Parker;
 import com.oracle.svm.core.thread.Parker.ParkerFactory;
 import com.oracle.svm.core.thread.PlatformThreads;
+import com.oracle.svm.core.thread.VMThreads.OSThreadHandle;
 import com.oracle.svm.core.util.UnsignedUtils;
 import com.oracle.svm.core.util.VMError;
 
+import jdk.graal.compiler.core.common.SuppressFBWarnings;
 import jdk.internal.misc.Unsafe;
 
 @AutomaticallyRegisteredImageSingleton(PlatformThreads.class)
@@ -121,7 +111,7 @@ protected boolean doStartThread(Thread thread, long stackSize) {
             ThreadStartData startData = prepareStart(thread, SizeOf.get(ThreadStartData.class));
 
             Pthread.pthread_tPointer newThread = UnsafeStackValue.get(Pthread.pthread_tPointer.class);
-            if (Pthread.pthread_create(newThread, attributes, PosixPlatformThreads.pthreadStartRoutine.getFunctionPointer(), startData) != 0) {
+            if (Pthread.pthread_create(newThread, attributes, threadStartRoutine.getFunctionPointer(), startData) != 0) {
                 undoPrepareStartOnError(thread, startData);
                 return false;
             }
@@ -187,32 +177,6 @@ protected void yieldCurrent() {
         Sched.sched_yield();
     }
 
-    private static final CEntryPointLiteral<CFunctionPointer> pthreadStartRoutine = CEntryPointLiteral.create(PosixPlatformThreads.class, "pthreadStartRoutine", ThreadStartData.class);
-
-    private static class PthreadStartRoutinePrologue implements CEntryPointOptions.Prologue {
-        private static final CGlobalData<CCharPointer> errorMessage = CGlobalDataFactory.createCString("Failed to attach a newly launched thread.");
-
-        @SuppressWarnings("unused")
-        @Uninterruptible(reason = "prologue")
-        static void enter(ThreadStartData data) {
-            int code = CEntryPointActions.enterAttachThread(data.getIsolate(), true, false);
-            if (code != CEntryPointErrors.NO_ERROR) {
-                CEntryPointActions.failFatally(code, errorMessage.get());
-            }
-        }
-    }
-
-    @CEntryPoint(include = CEntryPoint.NotIncludedAutomatically.class, publishAs = Publish.NotPublished)
-    @CEntryPointOptions(prologue = PthreadStartRoutinePrologue.class, epilogue = LeaveDetachThreadEpilogue.class)
-    static WordBase pthreadStartRoutine(ThreadStartData data) {
-        ObjectHandle threadHandle = data.getThreadHandle();
-        freeStartData(data);
-
-        threadStartRoutine(threadHandle);
-
-        return WordFactory.nullPointer();
-    }
-
     @Override
     protected void beforeThreadRun(Thread thread) {
         /* Complete the initialization of the thread, now that it is (nearly) running. */
@@ -255,7 +219,7 @@ public OSThreadHandle startThreadUnmanaged(CFunctionPointer threadRoutine, Point
                 return WordFactory.nullPointer();
             }
 
-            return (OSThreadHandle) newThread.read();
+            return newThread.read();
         } finally {
             Pthread.pthread_attr_destroy_no_transition(attributes);
         }
@@ -300,13 +264,6 @@ public void setUnmanagedThreadLocalValue(ThreadLocalKey key, WordBase value) {
         int resultCode = Pthread.pthread_setspecific((Pthread.pthread_key_t) key, (VoidPointer) value);
         PosixUtils.checkStatusIs0(resultCode, "pthread_setspecific(key, value): wrong arguments.");
     }
-
-    @Override
-    @SuppressWarnings("unused")
-    @Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)
-    public void closeOSThreadHandle(OSThreadHandle threadHandle) {
-        // pthread_t doesn't need closing
-    }
 }
 
 @TargetClass(Thread.class)

substratevm/src/com.oracle.svm.core.windows/src/com/oracle/svm/core/windows/WindowsPlatformThreads.java

@@ -24,18 +24,11 @@
  */
 package com.oracle.svm.core.windows;
 
-import org.graalvm.nativeimage.ObjectHandle;
 import org.graalvm.nativeimage.Platform;
 import org.graalvm.nativeimage.Platform.HOSTED_ONLY;
 import org.graalvm.nativeimage.Platforms;
-import org.graalvm.nativeimage.c.function.CEntryPoint;
-import org.graalvm.nativeimage.c.function.CEntryPoint.Publish;
-import org.graalvm.nativeimage.c.function.CEntryPointLiteral;
 import org.graalvm.nativeimage.c.function.CFunctionPointer;
-import org.graalvm.nativeimage.c.struct.RawField;
-import org.graalvm.nativeimage.c.struct.RawStructure;
 import org.graalvm.nativeimage.c.struct.SizeOf;
-import org.graalvm.nativeimage.c.type.CCharPointer;
 import org.graalvm.nativeimage.c.type.CIntPointer;
 import org.graalvm.nativeimage.c.type.VoidPointer;
 import org.graalvm.nativeimage.c.type.WordPointer;
@@ -44,24 +37,20 @@
 import org.graalvm.word.WordFactory;
 
 import com.oracle.svm.core.Uninterruptible;
-import com.oracle.svm.core.c.CGlobalData;
-import com.oracle.svm.core.c.CGlobalDataFactory;
-import com.oracle.svm.core.c.function.CEntryPointActions;
-import com.oracle.svm.core.c.function.CEntryPointErrors;
-import com.oracle.svm.core.c.function.CEntryPointOptions;
-import com.oracle.svm.core.c.function.CEntryPointSetup.LeaveDetachThreadEpilogue;
 import com.oracle.svm.core.feature.AutomaticallyRegisteredImageSingleton;
-import com.oracle.svm.core.graal.stackvalue.UnsafeStackValue;
 import com.oracle.svm.core.stack.StackOverflowCheck;
 import com.oracle.svm.core.thread.Parker;
 import com.oracle.svm.core.thread.Parker.ParkerFactory;
 import com.oracle.svm.core.thread.PlatformThreads;
+import com.oracle.svm.core.thread.VMThreads.OSThreadHandle;
 import com.oracle.svm.core.util.TimeUtils;
 import com.oracle.svm.core.util.VMError;
 import com.oracle.svm.core.windows.headers.Process;
 import com.oracle.svm.core.windows.headers.SynchAPI;
 import com.oracle.svm.core.windows.headers.WinBase;
 
+import jdk.graal.compiler.core.common.NumUtil;
+
 @AutomaticallyRegisteredImageSingleton(PlatformThreads.class)
 @Platforms(Platform.WINDOWS.class)
 public final class WindowsPlatformThreads extends PlatformThreads {
@@ -71,27 +60,21 @@ public final class WindowsPlatformThreads extends PlatformThreads {
 
     @Override
     protected boolean doStartThread(Thread thread, long stackSize) {
-        int threadStackSize = (int) stackSize;
-        int initFlag = Process.CREATE_SUSPENDED();
-
-        WindowsThreadStartData startData = prepareStart(thread, SizeOf.get(WindowsThreadStartData.class));
+        int threadStackSize = NumUtil.safeToUInt(stackSize);
 
+        int initFlag = 0;
         // If caller specified a stack size, don't commit it all at once.
         if (threadStackSize != 0) {
             initFlag |= Process.STACK_SIZE_PARAM_IS_A_RESERVATION();
         }
 
-        CIntPointer osThreadID = UnsafeStackValue.get(CIntPointer.class);
-        WinBase.HANDLE osThreadHandle = Process._beginthreadex(WordFactory.nullPointer(), threadStackSize,
-                        WindowsPlatformThreads.osThreadStartRoutine.getFunctionPointer(), startData, initFlag, osThreadID);
+        ThreadStartData startData = prepareStart(thread, SizeOf.get(ThreadStartData.class));
+        WinBase.HANDLE osThreadHandle = Process._beginthreadex(WordFactory.nullPointer(), threadStackSize, threadStartRoutine.getFunctionPointer(), startData, initFlag, WordFactory.nullPointer());
         if (osThreadHandle.isNull()) {
             undoPrepareStartOnError(thread, startData);
             return false;
         }
-        startData.setOSThreadHandle(osThreadHandle);
-
-        // Start the thread running
-        Process.ResumeThread(osThreadHandle);
+        WinBase.CloseHandle(osThreadHandle);
         return true;
     }
 
@@ -107,7 +90,7 @@ public OSThreadHandle startThreadUnmanaged(CFunctionPointer threadRoutine, Point
 
         WinBase.HANDLE osThreadHandle = Process.NoTransitions._beginthreadex(WordFactory.nullPointer(), stackSize,
                         threadRoutine, userData, initFlag, WordFactory.nullPointer());
-        return (PlatformThreads.OSThreadHandle) osThreadHandle;
+        return (OSThreadHandle) osThreadHandle;
     }
 
     @Override
@@ -177,51 +160,6 @@ protected void setNativeName(Thread thread, String name) {
     protected void yieldCurrent() {
         Process.SwitchToThread();
     }
-
-    @RawStructure
-    interface WindowsThreadStartData extends ThreadStartData {
-
-        @RawField
-        WinBase.HANDLE getOSThreadHandle();
-
-        @RawField
-        void setOSThreadHandle(WinBase.HANDLE osHandle);
-    }
-
-    private static final CEntryPointLiteral<CFunctionPointer> osThreadStartRoutine = CEntryPointLiteral.create(WindowsPlatformThreads.class, "osThreadStartRoutine", WindowsThreadStartData.class);
-
-    private static class OSThreadStartRoutinePrologue implements CEntryPointOptions.Prologue {
-        private static final CGlobalData<CCharPointer> errorMessage = CGlobalDataFactory.createCString("Failed to attach a newly launched thread.");
-
-        @SuppressWarnings("unused")
-        @Uninterruptible(reason = "prologue")
-        static void enter(WindowsThreadStartData data) {
-            int code = CEntryPointActions.enterAttachThread(data.getIsolate(), true, false);
-            if (code != CEntryPointErrors.NO_ERROR) {
-                CEntryPointActions.failFatally(code, errorMessage.get());
-            }
-        }
-    }
-
-    @CEntryPoint(include = CEntryPoint.NotIncludedAutomatically.class, publishAs = Publish.NotPublished)
-    @CEntryPointOptions(prologue = OSThreadStartRoutinePrologue.class, epilogue = LeaveDetachThreadEpilogue.class)
-    static WordBase osThreadStartRoutine(WindowsThreadStartData data) {
-        ObjectHandle threadHandle = data.getThreadHandle();
-        WinBase.HANDLE osThreadHandle = data.getOSThreadHandle();
-        freeStartData(data);
-
-        try {
-            threadStartRoutine(threadHandle);
-        } finally {
-            /*
-             * Note that there is another handle to the thread stored in VMThreads.OSThreadHandleTL.
-             * This is necessary to ensure that the operating system does not release the thread
-             * resources too early.
-             */
-            WinBase.CloseHandle(osThreadHandle);
-        }
-        return WordFactory.nullPointer();
-    }
 }
 
 /**

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/JavaMainWrapper.java

@@ -35,7 +35,6 @@
 import java.util.List;
 import java.util.function.BooleanSupplier;
 
-import jdk.graal.compiler.word.Word;
 import org.graalvm.nativeimage.CurrentIsolate;
 import org.graalvm.nativeimage.ImageSingletons;
 import org.graalvm.nativeimage.Isolate;
@@ -75,12 +74,15 @@
 import com.oracle.svm.core.thread.PlatformThreads;
 import com.oracle.svm.core.thread.ThreadListenerSupport;
 import com.oracle.svm.core.thread.VMThreads;
+import com.oracle.svm.core.thread.VMThreads.OSThreadHandle;
 import com.oracle.svm.core.util.CounterSupport;
 import com.oracle.svm.core.util.UserError;
 import com.oracle.svm.core.util.VMError;
 import com.oracle.svm.util.ClassUtil;
 import com.oracle.svm.util.ReflectionUtil;
 
+import jdk.graal.compiler.word.Word;
+
 @InternalVMMethod
 public class JavaMainWrapper {
     /*
@@ -309,7 +311,7 @@ private static int doRunInNewThread(int argc, CCharPointerPointer argv) {
         MAIN_ISOLATE_PARAMETERS.get().setArgc(argc);
         MAIN_ISOLATE_PARAMETERS.get().setArgv(argv);
         long stackSize = SubstrateOptions.StackSize.getHostedValue();
-        PlatformThreads.OSThreadHandle osThreadHandle = PlatformThreads.singleton().startThreadUnmanaged(RUN_MAIN_ROUTINE.get(), WordFactory.nullPointer(), (int) stackSize);
+        OSThreadHandle osThreadHandle = PlatformThreads.singleton().startThreadUnmanaged(RUN_MAIN_ROUTINE.get(), WordFactory.nullPointer(), (int) stackSize);
         if (osThreadHandle.isNull()) {
             CEntryPointActions.failFatally(1, START_THREAD_UNMANAGED_ERROR_MESSAGE.get());
             return 1;

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/thread/PlatformThreads.java

@@ -59,10 +59,12 @@
 import org.graalvm.nativeimage.Platforms;
 import org.graalvm.nativeimage.UnmanagedMemory;
 import org.graalvm.nativeimage.c.function.CEntryPoint;
+import org.graalvm.nativeimage.c.function.CEntryPointLiteral;
 import org.graalvm.nativeimage.c.function.CFunctionPointer;
 import org.graalvm.nativeimage.c.function.CodePointer;
 import org.graalvm.nativeimage.c.struct.RawField;
 import org.graalvm.nativeimage.c.struct.RawStructure;
+import org.graalvm.nativeimage.c.type.CCharPointer;
 import org.graalvm.nativeimage.c.type.WordPointer;
 import org.graalvm.word.ComparableWord;
 import org.graalvm.word.Pointer;
@@ -79,6 +81,12 @@
 import com.oracle.svm.core.annotate.Alias;
 import com.oracle.svm.core.annotate.TargetClass;
 import com.oracle.svm.core.annotate.TargetElement;
+import com.oracle.svm.core.c.CGlobalData;
+import com.oracle.svm.core.c.CGlobalDataFactory;
+import com.oracle.svm.core.c.function.CEntryPointActions;
+import com.oracle.svm.core.c.function.CEntryPointErrors;
+import com.oracle.svm.core.c.function.CEntryPointOptions;
+import com.oracle.svm.core.c.function.CEntryPointSetup;
 import com.oracle.svm.core.heap.Heap;
 import com.oracle.svm.core.heap.ReferenceHandler;
 import com.oracle.svm.core.heap.ReferenceHandlerThread;
@@ -93,6 +101,7 @@
 import com.oracle.svm.core.nodes.CFunctionPrologueNode;
 import com.oracle.svm.core.stack.StackFrameVisitor;
 import com.oracle.svm.core.stack.StackOverflowCheck;
+import com.oracle.svm.core.thread.VMThreads.OSThreadHandle;
 import com.oracle.svm.core.thread.VMThreads.StatusSupport;
 import com.oracle.svm.core.threadlocal.FastThreadLocal;
 import com.oracle.svm.core.threadlocal.FastThreadLocalFactory;
@@ -122,6 +131,8 @@ public static PlatformThreads singleton() {
         return ImageSingletons.lookup(PlatformThreads.class);
     }
 
+    protected static final CEntryPointLiteral<CFunctionPointer> threadStartRoutine = CEntryPointLiteral.create(PlatformThreads.class, "threadStartRoutine", ThreadStartData.class);
+
     /** The platform {@link java.lang.Thread} for the {@link IsolateThread}. */
     static final FastThreadLocalObject<Thread> currentThread = FastThreadLocalFactory.createObject(Thread.class, "PlatformThreads.currentThread").setMaxOffset(FastThreadLocal.BYTE_OFFSET);
 
@@ -539,7 +550,7 @@ public void setUnmanagedThreadLocalValue(ThreadLocalKey key, WordBase value) {
     @SuppressWarnings("unused")
     @Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)
     public void closeOSThreadHandle(OSThreadHandle threadHandle) {
-        throw VMError.shouldNotReachHere("Shouldn't call PlatformThreads.closeOSThreadHandle directly.");
+        /* On most platforms, OS thread handles don't need to be closed. */
     }
 
     static final Method FORK_JOIN_POOL_TRY_TERMINATE_METHOD;
@@ -762,6 +773,16 @@ void startThread(Thread thread, long stackSize) {
      */
     protected abstract boolean doStartThread(Thread thread, long stackSize);
 
+    @CEntryPoint(include = CEntryPoint.NotIncludedAutomatically.class, publishAs = CEntryPoint.Publish.NotPublished)
+    @CEntryPointOptions(prologue = ThreadStartRoutinePrologue.class, epilogue = CEntryPointSetup.LeaveDetachThreadEpilogue.class)
+    protected static WordBase threadStartRoutine(ThreadStartData data) {
+        ObjectHandle threadHandle = data.getThreadHandle();
+        freeStartData(data);
+
+        threadStartRoutine(threadHandle);
+        return WordFactory.nullPointer();
+    }
+
     @SuppressFBWarnings(value = "Ru", justification = "We really want to call Thread.run and not Thread.start because we are in the low-level thread start routine")
     protected static void threadStartRoutine(ObjectHandle threadHandle) {
         Thread thread = ObjectHandles.getGlobal().get(threadHandle);
@@ -1194,8 +1215,17 @@ static void blockedOn(Target_sun_nio_ch_Interruptible b) {
         }
     }
 
-    @RawStructure
-    public interface OSThreadHandle extends PointerBase {
+    protected static class ThreadStartRoutinePrologue implements CEntryPointOptions.Prologue {
+        private static final CGlobalData<CCharPointer> errorMessage = CGlobalDataFactory.createCString("Failed to attach a newly launched thread.");
+
+        @SuppressWarnings("unused")
+        @Uninterruptible(reason = "prologue")
+        static void enter(ThreadStartData data) {
+            int code = CEntryPointActions.enterAttachThread(data.getIsolate(), true, false);
+            if (code != CEntryPointErrors.NO_ERROR) {
+                CEntryPointActions.failFatally(code, errorMessage.get());
+            }
+        }
     }
 
     public interface ThreadLocalKey extends ComparableWord {

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/thread/VMThreads.java

@@ -362,9 +362,11 @@ public void detachThread(IsolateThread thread) {
         assert thread.equal(CurrentIsolate.getCurrentThread()) : "Cannot detach different thread with this method";
 
         // read thread local data (can't be accessed further below as the IsolateThread is freed)
+        OSThreadHandle threadHandle = OSThreadHandleTL.get(thread);
         OSThreadHandle nextOsThreadToCleanup = WordFactory.nullPointer();
-        if (wasStartedByCurrentIsolate(thread)) {
-            nextOsThreadToCleanup = OSThreadHandleTL.get(thread);
+        boolean wasStartedByCurrentIsolate = wasStartedByCurrentIsolate(thread);
+        if (wasStartedByCurrentIsolate) {
+            nextOsThreadToCleanup = threadHandle;
         }
 
         threadExit(thread);
@@ -401,6 +403,10 @@ public void detachThread(IsolateThread thread) {
             THREAD_MUTEX.unlockNoTransitionUnspecifiedOwner();
         }
 
+        if (!wasStartedByCurrentIsolate) {
+            /* If a thread was attached, we need to free its thread handle. */
+            PlatformThreads.singleton().closeOSThreadHandle(threadHandle);
+        }
         cleanupExitedOsThread(threadToCleanup);
     }