Feature #17369
closedIntroduce non-blocking `Process.wait`, `Kernel.system` and related methods.
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) over 3 years 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) over 3 years 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) over 3 years 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.
Updated by ioquatix (Samuel Williams) over 3 years ago
- Description updated (diff)
Updated by ioquatix (Samuel Williams) over 3 years ago
Non-blocking Process.wait
has been merged.
Updated by naruse (Yui NARUSE) over 3 years 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) over 3 years ago
I am OK with Process::Status.wait
. As far as I've heard the code quality needs upgrade.
Matz.
Updated by ioquatix (Samuel Williams) over 3 years 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?
Updated by jeremyevans0 (Jeremy Evans) over 3 years ago
- Tracker changed from Bug to Feature
- Backport deleted (
2.5: UNKNOWN, 2.6: UNKNOWN, 2.7: UNKNOWN)
Updated by ioquatix (Samuel Williams) about 3 years ago
See https://github.com/ruby/ruby/pull/4595 which implements non-blocking Kernel#system
.
Updated by ioquatix (Samuel Williams) almost 3 years ago
The implementation is completed.
However, some parts are still pretty messy, including leaking $?
process status.
We need to make $?
fiber local.
irb(main):012:0> Fiber.new{system("false")}.resume; pp $?
#<Process::Status: pid 628235 exit 1>
=> #<Process::Status: pid 628235 exit 1>
irb(main):013:0> Fiber.new{system("true")}.resume; pp $?
#<Process::Status: pid 628241 exit 0>
=> #<Process::Status: pid 628241 exit 0>
However this might cause some issues in existing code.
Should we consider to deprecate $?
?
Updated by Eregon (Benoit Daloze) almost 3 years ago
I think making $?
Fiber-local makes sense, and unlikely to break anything.
I don't see the need to deprecate $?
, and it's certainly not worth the cost to migrate existing code to some other way to the get the status.
Updated by nobu (Nobuyoshi Nakada) almost 3 years ago
Please file a new issue for fiber-local $?
.
Updated by nobu (Nobuyoshi Nakada) almost 3 years ago
- Status changed from Assigned to Closed