[GR-14575] Fix race in Thread.list.

PullRequest: truffleruby/713

Author: eregon

spec/ruby/core/thread/list_spec.rb

@@ -35,8 +35,21 @@
       t.join
     end
   end
-end
 
-describe "Thread.list" do
-  it "needs to be reviewed for spec completeness"
+  it "returns instances of Thread and not null or nil values" do
+    spawner = Thread.new do
+      Array.new(100) do
+        Thread.new {}
+      end
+    end
+
+    while spawner.alive?
+      Thread.list.each { |th|
+        th.should be_kind_of(Thread)
+      }
+    end
+
+    threads = spawner.value
+    threads.each(&:join)
+  end
 end

src/main/java/org/truffleruby/core/thread/ThreadManager.java

@@ -148,6 +148,7 @@ public DynamicObject createBootThread(String info) {
 
     public DynamicObject createThread(DynamicObject rubyClass, AllocateObjectNode allocateObjectNode) {
         final DynamicObject currentGroup = Layouts.THREAD.getThreadGroup(getCurrentThread());
+        assert currentGroup != null;
         final DynamicObject thread = allocateObjectNode.allocate(rubyClass,
                 packThreadFields(currentGroup, "<uninitialized>"));
         setFiberManager(thread);
@@ -156,6 +157,7 @@ public DynamicObject createThread(DynamicObject rubyClass, AllocateObjectNode al
 
     public DynamicObject createForeignThread() {
         final DynamicObject currentGroup = Layouts.THREAD.getThreadGroup(rootThread);
+        assert currentGroup != null;
         final DynamicObject thread = context.getCoreLibrary().getThreadFactory().newInstance(
                 packThreadFields(currentGroup, "<foreign thread>"));
         setFiberManager(thread);
@@ -624,7 +626,8 @@ private void doKillOtherThreads() {
 
     @TruffleBoundary
     public Object[] getThreadList() {
-        return runningRubyThreads.toArray(new Object[runningRubyThreads.size()]);
+        // This must not pre-allocate an array, or it could contain null's.
+        return runningRubyThreads.toArray();
     }
 
     @TruffleBoundary

src/main/java/org/truffleruby/core/thread/ThreadNodes.java

@@ -40,9 +40,16 @@
  */
 package org.truffleruby.core.thread;
 
-import java.util.concurrent.CountDownLatch;
-import java.util.concurrent.TimeUnit;
-
+import com.oracle.truffle.api.CompilerDirectives;
+import com.oracle.truffle.api.CompilerDirectives.CompilationFinal;
+import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
+import com.oracle.truffle.api.dsl.Cached;
+import com.oracle.truffle.api.dsl.Specialization;
+import com.oracle.truffle.api.frame.VirtualFrame;
+import com.oracle.truffle.api.nodes.Node;
+import com.oracle.truffle.api.object.DynamicObject;
+import com.oracle.truffle.api.profiles.BranchProfile;
+import com.oracle.truffle.api.source.SourceSection;
 import org.jcodings.specific.USASCIIEncoding;
 import org.jcodings.specific.UTF8Encoding;
 import org.truffleruby.Layouts;
@@ -76,16 +83,8 @@
 import org.truffleruby.language.objects.shared.SharedObjects;
 import org.truffleruby.language.yield.YieldNode;
 
-import com.oracle.truffle.api.CompilerDirectives;
-import com.oracle.truffle.api.CompilerDirectives.CompilationFinal;
-import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
-import com.oracle.truffle.api.dsl.Cached;
-import com.oracle.truffle.api.dsl.Specialization;
-import com.oracle.truffle.api.frame.VirtualFrame;
-import com.oracle.truffle.api.nodes.Node;
-import com.oracle.truffle.api.object.DynamicObject;
-import com.oracle.truffle.api.profiles.BranchProfile;
-import com.oracle.truffle.api.source.SourceSection;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
 
 @CoreClass("Thread")
 public abstract class ThreadNodes {
@@ -244,6 +243,18 @@ private DynamicObject getNeverSymbol() {
 
     }
 
+    @Primitive(name = "thread_allocate")
+    public abstract static class ThreadAllocateNode extends PrimitiveArrayArgumentsNode {
+
+        @Specialization
+        public DynamicObject allocate(
+                DynamicObject rubyClass,
+                @Cached("create()") AllocateObjectNode allocateObjectNode) {
+            return getContext().getThreadManager().createThread(rubyClass, allocateObjectNode);
+        }
+
+    }
+
     @Primitive(name = "thread_initialized?")
     public abstract static class ThreadIsInitializedNode extends PrimitiveArrayArgumentsNode {
 
@@ -487,18 +498,6 @@ public DynamicObject setAbortOnException(DynamicObject self, boolean abortOnExce
 
     }
 
-    @Primitive(name = "thread_allocate")
-    public abstract static class ThreadAllocateNode extends PrimitiveArrayArgumentsNode {
-
-        @Specialization
-        public DynamicObject allocate(
-                DynamicObject rubyClass,
-                @Cached("create()") AllocateObjectNode allocateObjectNode) {
-            return getContext().getThreadManager().createThread(rubyClass, allocateObjectNode);
-        }
-
-    }
-
     @CoreMethod(names = "list", onSingleton = true)
     public abstract static class ListNode extends CoreMethodArrayArgumentsNode {