Bug #10231: Process.detach(pid) defines new singleton classes every call

• Status: Closed
• Assignee: normalperson (Eric Wong)
• Target version: 2.2.0
• ruby -v: Any version with Process.detach
• Backport: 2.0.0: UNKNOWN, 2.1: UNKNOWN

[headius (Charles Nutter)]

The logic for Process.detach(pid) adds a singleton "pid" method to the thread it returns for every call. This is bad for method caches (MRI still flushes them all for this, I believe) and memory churn (singleton classes are not small).

The offending line of code is here: https://github.com/ruby/ruby/blob/trunk/process.c#L1041

I would suggest that Process.detach should return a subclass of Thread that has the pid method defined ahead of time.

It also stores the value in thread local storage, rather than as an instance variable. I'm not sure why.

[normalperson (Eric Wong)]

[e-mail] wrote:

I would suggest that Process.detach should return a subclass of Thread
that has the pid method defined ahead of time.

Fair enough.

It also stores the value in thread local storage, rather than as an
instance variable. I'm not sure why.

It seems to be documented for Open3, so it might be spec and we can't
change it.

Fwiw, I do not like Process.detach and prefer to use trap(:CHLD)+waitpid
instead in applications. Unfortunately trap is not library-friendly.

[normalperson (Eric Wong)]

I haven't checked the inits.c ordering change closely, but
make check and test-rubyspec passes:

diff --git a/inits.c b/inits.c
index fe0aade..da8cfb1 100644
--- a/inits.c
+++ b/inits.c
@@ -46,7 +46,6 @@ rb_call_inits(void)
     CALL(Time);
     CALL(Random);
     CALL(signal);
-    CALL(process);
     CALL(load);
     CALL(Proc);
     CALL(Binding);
@@ -56,6 +55,7 @@ rb_call_inits(void)
     CALL(VM);
     CALL(ISeq);
     CALL(Thread);
+    CALL(process);
     CALL(Cont);
     CALL(Rational);
     CALL(Complex);
diff --git a/process.c b/process.c
index 8bb52f7..5bf59ac 100644
--- a/process.c
+++ b/process.c
@@ -1007,6 +1007,8 @@ proc_waitall(void)
     return result;
 }
 
+static VALUE rb_cWaiter;
+
 static inline ID
 id_pid(void)
 {
@@ -1038,7 +1040,7 @@ rb_detach_process(rb_pid_t pid)
 {
     VALUE watcher = rb_thread_create(detach_process_watcher, (void*)(VALUE)pid);
     rb_thread_local_aset(watcher, id_pid(), PIDT2NUM(pid));
-    rb_define_singleton_method(watcher, "pid", detach_process_pid, 0);
+    RBASIC_SET_CLASS(watcher, rb_cWaiter);
     return watcher;
 }
 
@@ -7516,6 +7518,10 @@ Init_process(void)
     rb_define_module_function(rb_mProcess, "waitall", proc_waitall, 0);
     rb_define_module_function(rb_mProcess, "detach", proc_detach, 1);
 
+    rb_cWaiter = rb_define_class_under(rb_mProcess, "Waiter", rb_cThread);
+    rb_undef_method(CLASS_OF(rb_cWaiter), "new");
+    rb_define_method(rb_cWaiter, "pid", detach_process_pid, 0);
+
     rb_cProcessStatus = rb_define_class_under(rb_mProcess, "Status", rb_cObject);
     rb_undef_method(CLASS_OF(rb_cProcessStatus), "new");
 
diff --git a/test/ruby/test_process.rb b/test/ruby/test_process.rb
index 608d663..a3fa36e 100644
--- a/test/ruby/test_process.rb
+++ b/test/ruby/test_process.rb
@@ -1965,4 +1965,10 @@ EOS
       runner.close
     end
   end if defined?(fork)
+
+  def test_process_detach
+    pid = fork {}
+    th = Process.detach(pid)
+    assert_equal pid, th.pid
+  end if defined?(fork)
 end

--
EW

[headius (Charles Nutter)]

Eric Wong wrote:

I haven't checked the inits.c ordering change closely, but
make check and test-rubyspec passes:

+1 This is how I'd fix it.

[nobu (Nobuyoshi Nakada)]

• Category set to core
• Assignee set to normalperson (Eric Wong)
• Target version set to 2.2.0
• Status changed from Open to Assigned

Probably, you can use rb_undef_alloc_func.
And should join the waiter thread.

• Status changed from Assigned to Closed
• % Done changed from 0 to 100

Applied in changeset r47561.

---

Process.detach: avoid singleton class creation

• process.c (Init_process): subclass Thread as Process::Waiter
  (rb_detach_process): use Process::Waiter instead of singleton class

• test/ruby/test_process.rb (test_process_detach): new test

• inits.c (rb_call_inits): call Init_Thread before Init_process to
  ensure Process::Waiter may be a subclass of Thread

Thanks to headius for reporting [Bug #10231]
Thanks to nobu for review of my initial patch.

[normalperson (Eric Wong)]

[e-mail] wrote:

Eric Wong wrote:

I haven't checked the inits.c ordering change closely, but
make check and test-rubyspec passes:

+1 This is how I'd fix it.

Committed with nobu's comments as r47561
Process.detach: avoid singleton class creation

A NEWS entry may be appropriate as well.

I prefer deliberately leaving out Process::Waiter and Thread references,
since I think we may be able to avoid Thread creation entirely in the
future (not sure if it's worth the effort).

--- a/NEWS
+++ b/NEWS
@@ -113,6 +113,8 @@ with all sufficient information, see the ChangeLog file.
* Process execution methods such as Process.spawn opens the file in write
mode for redirect from [:out, :err].
Before Ruby 2.2, it was opened in read mode.

• • Process.detach returns the same object class instead of creating a
• new singleton class every call.

=== Stdlib updates (outstanding ones only)

[headius (Charles Nutter)]

Eric Wong wrote:

I prefer deliberately leaving out Process::Waiter and Thread references,
since I think we may be able to avoid Thread creation entirely in the
future (not sure if it's worth the effort).

FWIW, JVM always does a waiter thread to avoid multiple calls to waitpid (which as you know returns results exactly once). The waiter thread is not unusual in that regard.

It's probably possible to avoid the thread and make concurrent waiters just wait on a mutex too...

def wait_for_process
@mutex.lock
@result ||= waitpid
ensure
@mutex.unlock
end

Not sure what wrenches this throws into UNIX-style process management, though.

Note that JVM's subprocess logic also has threads draining the child's streams into an in-memory buffer. I do NOT recommend that. In JRuby 9k we no longer use the JVM's subprocess logic.

[normalperson (Eric Wong)]

[e-mail] wrote:

FWIW, JVM always does a waiter thread to avoid multiple calls to
waitpid (which as you know returns results exactly once). The waiter
thread is not unusual in that regard.

Right. The problem I want to solve is when independently-maintained
pieces of code have conflicting calls to waitpid:

th = Thread.new { Process.waitpid(-1) }

p Process.waitpid(fork {})
p th.value

The above behaves inconsistently depending on scheduling. Presumably
the thread waiting on a single pid should beat the thread waiting on all
threads. And it may not be solvable for waiting on process groups...

So the above combination of waitpid calls is likely broken no matter
what.

[headius (Charles Nutter)]

Eric Wong wrote:

The above behaves inconsistently depending on scheduling. Presumably
the thread waiting on a single pid should beat the thread waiting on all
threads. And it may not be solvable for waiting on process groups...

So the above combination of waitpid calls is likely broken no matter
what.

Yeah, I don't think there's a solution for that unless you pipe ALL waitpid calls through the same logic that waits on first caller for that pid...and then there's process groups, as you say.