Bug #20393: `after_fork_ruby` clears all pending interrupts for both parent and child process.

• Status: Closed
• Assignee: ioquatix (Samuel Williams)
• Target version: -
• Backport: 3.0: REQUIRED, 3.1: REQUIRED, 3.2: DONE, 3.3: DONE

[ioquatix (Samuel Williams)]

In the following program, the behaviour of the parent process is affected by whether Process.fork is invoked or not.

Thread.handle_interrupt(RuntimeError => :never) do
  Thread.current.raise(RuntimeError, "Queued error")
  
  puts "Pending interrupt: #{Thread.pending_interrupt?}" # true
  
  pid = Process.fork do
    puts "Pending interrupt (child process): #{Thread.pending_interrupt?}"
    Thread.handle_interrupt(RuntimeError => :immediate){}
  end
  
  _, status = Process.waitpid2(pid)
  puts "Child process status: #{status.inspect}"
  
  puts "Pending interrupt: #{Thread.pending_interrupt?}" # false
end

puts "Exiting..."

I don't think the parent process pending interrupts should be cleared by after_fork_ruby:

static void
after_fork_ruby(rb_pid_t pid)
{
    rb_threadptr_pending_interrupt_clear(GET_THREAD());
    if (pid == 0) {
        // child
        clear_pid_cache();
        rb_thread_atfork();
    }
    else {
        // parent
        after_exec();
    }
}

How about this implementation:

static void
after_fork_ruby(rb_pid_t pid)
{
    if (pid == 0) {
        // child
        rb_threadptr_pending_interrupt_clear(GET_THREAD());
        clear_pid_cache();
        rb_thread_atfork();
    }
    else {
        // parent
        after_exec();
    }
}

cc @ko1 (Koichi Sasada)

[ioquatix (Samuel Williams)]

@nobu (Nobuyoshi Nakada) can you please review https://github.com/ruby/ruby/pull/10365 thanks!

Some more background found by @mame (Yusuke Endoh):

• Originally introduced in https://github.com/ruby/ruby/commit/2d5061bd98a59fc1b9a477074f8f7a3500db8342. It looks like the bug exists here too, as GET_THREAD()->thrown_errinfo = 0 would be cleared both in the child and parent process.
• Modified here to use a list of pending interrupts: https://github.com/ruby/ruby/commit/28144433b2f951279552b39bc358769a5267e26a
• Renamed from async_errinfo to pending_interrupts: https://github.com/ruby/ruby/commit/0f9b33c793f225c1b817d73e5c915050c429edc4

[ioquatix (Samuel Williams)]

• Related to Bug #885: Thread.new{fork{}} added

[ioquatix (Samuel Williams)]

• Status changed from Open to Closed

Nobu approved this change on the PR, so I've merged it: https://github.com/ruby/ruby/commit/a7ff264477105b5dc0ade6facad4176a1b73df0b

I'll introduce a separate PR to add the test to ruby-spec.

[k0kubun (Takashi Kokubun)]

• Backport changed from 3.0: REQUIRED, 3.1: REQUIRED, 3.2: REQUIRED, 3.3: REQUIRED to 3.0: REQUIRED, 3.1: REQUIRED, 3.2: REQUIRED, 3.3: DONE

ruby_3_3 6e46a363a8f29d93cf6992805ee67d029cea030f merged revision(s) a7ff264477105b5dc0ade6facad4176a1b73df0b.

[nagachika (Tomoyuki Chikanaga)]

• Backport changed from 3.0: REQUIRED, 3.1: REQUIRED, 3.2: REQUIRED, 3.3: DONE to 3.0: REQUIRED, 3.1: REQUIRED, 3.2: DONE, 3.3: DONE

ruby_3_2 5577e5d396cc8f062833b67d6280db6cc8501e7a merged revision(s) a7ff264477105b5dc0ade6facad4176a1b73df0b.

[ioquatix (Samuel Williams)]

Thanks!