Project

General

Profile

Actions

Feature #17369

open

Introduce non-blocking `Process.wait`, `Kernel.system` and related methods.

Added by ioquatix (Samuel Williams) 8 months ago. Updated about 1 month ago.

Status:
Assigned
Priority:
Normal
Target version:
-
[ruby-core:101250]

Description

https://github.com/ruby/ruby/pull/3853

This PR introduces optional hooks to the scheduler interface for handling Process.wait, Kernel.system and other related methods (waitpid, wait2, etc).

It funnels all methods through a new interface Process::Status.wait which is almost identical to Process.wait except for several key differences:

  • The return value is a single instance of Process::Status.
  • It does not set thread local $?.

This is necessary for keeping the scheduler interface simple (and side effects are generally bad anyway).

Updated by Eregon (Benoit Daloze) 8 months ago

Does such code still work, with a scheduler?

`echo foo`
p $? # => #<Process::Status: pid 43525 exit 0>

If not, it seems a significant problem, as existing code would break with a scheduler.

Given the implementation in the test scheduler:

  def process_wait(pid, flags)
    # This is a very simple way to implement a non-blocking wait:
    Thread.new do
      Process::Status.wait(pid, flags)
    end.join
  end

It sounds like you would need a way to set $? on the current Thread.
So that $? can be set for the caller.
I think that's fine to add.

I think $? should be Fiber-local, probably it's thread-local only for historic reasons.
Otherwise, just switching between Fibers (e.g., on IO) would expose the $? of other Fibers, which will lead to bugs.
I expect that change to cause extremely few compatibility issues ($~, etc are already fiber-local + frame-local).

Updated by ioquatix (Samuel Williams) 8 months ago

Does such code still work, with a scheduler?

Yes.

It sounds like you would need a way to set $? on the current Thread.

Nope, it's handled by Process.wait and so on.

Otherwise, just switching between Fibers (e.g., on IO) would expose the $? of other Fibers, which will lead to bugs.

Agree, but we can't change this without potentially breaking existing code.

Also, is it okay that Process.last_status and Process.last_status= (hypothetical) are fiber local? Because Matz already said he was against class attributes that are actually fiber local (even if I agree in theory, excluding the fact that this is a breaking change).

I expect that change to cause extremely few compatibility issues ($~, etc are already fiber-local + frame-local).

Great, if Matz can approve the change, then we can implement it, but it's separate from this PR, since this PR just makes the existing interface non-blocking.

Updated by Eregon (Benoit Daloze) 8 months ago

I clarified with ioquatix (Samuel Williams), the code above should be end.value so it returns the Process::Status and system still sets it.
Then the change sounds good to me.

Actions #4

Updated by ioquatix (Samuel Williams) 8 months ago

  • Description updated (diff)

Updated by ioquatix (Samuel Williams) 8 months ago

Non-blocking Process.wait has been merged.

Updated by naruse (Yui NARUSE) 8 months ago

Is this feature discussed with ko1 and nobu?
Also I suspect Matz's approval is required for this change.

Updated by matz (Yukihiro Matsumoto) 8 months ago

I am OK with Process::Status.wait. As far as I've heard the code quality needs upgrade.

Matz.

Actions #8

Updated by naruse (Yui NARUSE) 8 months ago

  • Target version set to 3.0
Actions #9

Updated by naruse (Yui NARUSE) 7 months ago

  • Target version deleted (3.0)

Updated by ioquatix (Samuel Williams) 7 months ago

We introduced experimental feature and implemented non-blocking hook for Ruby 3.

More work is required here, but we didn't make it in time for Ruby 3.0 - so we marked it as experimental.

We also need to implement rb_f_system in terms of rb_process_status_wait. Can someone else help with this?

Actions #11

Updated by jeremyevans0 (Jeremy Evans) 6 months ago

  • Backport deleted (2.5: UNKNOWN, 2.6: UNKNOWN, 2.7: UNKNOWN)
  • Tracker changed from Bug to Feature

Updated by ioquatix (Samuel Williams) about 1 month ago

See https://github.com/ruby/ruby/pull/4595 which implements non-blocking Kernel#system.

Actions

Also available in: Atom PDF