I guess the caller always knows pid, so another strategy would be to only return the wstatus as an Integer from the scheduler callback.

I guess the caller always knows pid, so another strategy would be to only return the wstatus as an Integer from the scheduler callback.

Unfortunately I don't think it's enough information to implement front end interfaces.

It turns out that the native process_wait hook doesn't need this. However, it might still be nice to introduce it and tidy up the way Process::Status is used/created.

I ended up rb_funcalling Process::Status.wait which was sufficient. However, we could be slightly more efficient if we could directly create status objects.

socketry/io-event@f24122a

I would say, the public C interface in Ruby is pretty hard to use correctly/seems patchy/messy. Could do with a good refactor.

I ended up rb_funcalling Process::Status.wait which was sufficient. However, we could be slightly more efficient if we could directly create status objects.

I think it's good enough and that seems the best abstraction for this, and avoids exposing any internal details of Process::Status.

I would say, the public C interface in Ruby is pretty hard to use correctly/seems patchy/messy. Could do with a good refactor.

Would need some specifics, but that's likely best discussed on its own.

If io_uring implements waitpid this will become necessary.

It seems like EventMachine would like this interface. cc @sodabrew

Was there a specific reason in mind for originally removing Process::Status.new here? Edit: nevermind, come to think of it and looking at the history, this has been removed since forever.

The previous MRI code used two hidden instance variables, so EventMachine was taking advantage of this to create an object of class Process::Status and manually set the pid and status:

    rb_ivar_set(proc_status, rb_intern_const("status"), INT2FIX(status));
    rb_ivar_set(proc_status, rb_intern_const("pid"), INT2FIX(pid));

Rubinius did nearly the same but with a visible variable, so the EventMachine code is:

    rb_iv_set(proc_status, "@pid", INT2FIX(pid));

The new Process::Status in Ruby 3 is using a C structure so @MSP-Greg attempted the approach to copying the structure definitions to create a Process::Status that is identical under the hood but separately defined in eventmachine/eventmachine#948. This bumps into a C++ compiler issue for older versions of g++, which surely with some workarounds we might solve, but is a continued effort to hack around MRI's lack of a clear interface for creating a Process::Status object that can respond to its usual methods.

Given that Ruby 3.0 is already released, we may need to proceed with the identical struct approach but at the risk of breaking in weird ways if anybody changes struct rb_process_status in a Ruby point release.

Thinking again about this, I think it could be fine to add, with #4563 (comment).

I'm still unsure why Process::Status even captures the errno, is anything using that?
If not it seems better to clean it up before making it a required argument of rb_process_status_new().

To clarify, for those who haven't looked at the EventMachine (EM) discussion, 'bumps into a C++ compiler issue for older versions of g++' happens with Ubuntu-18.04.

CI in EM was only testing with 'latest' of Ubuntu, macOS, and Windows, and all passed. Someone pointed out that the code did not work with Ubuntu-18.04. Or, if a repo is using c++, testing against Ubuntu-18.04 is a very good idea.

I'm still unsure why Process::Status even captures the errno, is anything using that?

Yes it's used for propagating the error.

If this is a big problem we can potentially backport it to 3.x - it's almost a bug if you have to copy the struct layout.

@MSP-Greg one option to consider - do you call waitpid in your own code? Can you replace that with rb_process_status_wait?

Yes it's used for propagating the error.

Could you give more details?
I don't see any method of Process::Status exposing the errno, all seem to act on the wstatus and nothing else.

I don't see any method of Process::Status exposing the errno, all seem to act on the wstatus and nothing else.

It's for rb_waitpid(), where is sets errno:

It's from #3998

@sodabrew @MSP-Greg rb_process_status_new seems already not-static before this PR.
So I think declaring VALUE rb_process_status_new(rb_pid_t pid, int status, int error); and then using that should work, even before this PR.
Duplicating the internal struct is super brittle and risky, don't do that.

Looks good to me now, but all instances should be frozen once initialized.

I don't agree with this. I don't know why it should be frozen. Can you give me a reason why?

Not sure what C extension would use that, but I guess best to keep it for compatibility.

It's part of the interface required by consumers of Process::Status because they check it to create exceptions in some cases.

I don't agree with this. I don't know why it should be frozen. Can you give me a reason why?

My POV is all classes ('s instances) which can be frozen should be frozen.
All Process::Status instances are frozen since Ruby 3 (fact: ruby -e 'system "echo"; p $?.frozen?' => true).
There is no good reason to make it mutable (after initialize), is it?
Do you actually want to store something there?

I am fine if we follow a general model of immutability so that the user is not surprised that .new returns a frozen instance. I believe in the past it was discussed something like .new_frozen.

However, I am strongly against calling #freeze from #initialize since it subtly breaks sub-classes and the decision to do so seems totally adhoc. Why this class and not some other class? The exception to the rule is confusing for users.

If we want to do this, it should be in .new but I don't want to be adhoc about the implementation. Personally, my expectation around this is that Object.new returns a mutable instance, and there are few, if any, cases where this isn't true. We discussed Range and Pathname, both which I think are poor examples personally since they could both benefit from local mutability to reduce allocation overhead.

Finally, I don't see any particular reason why Process::Status should be frozen - not only does it not make any tangible difference to the base class implementation, I don't even believe we have any mutable methods (attributes, etc). Maybe we should add attribute setters just so we can freeze the instance and they can't be used. Yolo.

For context, the addition of errno to Process::Status was done in #3998

And #3671 is what made Process::Status frozen, and I think we should not regress there.
I'm totally fine with defining Process::Status.new (instead of initialize) and make that freeze the instance, it seems a good way too.

@eregon

rb_process_status_new seems already not-static before this PR.
So I think declaring VALUE rb_process_status_new(rb_pid_t pid, int status, int error); and then using that should work, even before this PR.

I tried this but it didn't work for me. The symbol was not found at runtime on MacOS.

I tried this but it didn't work for me. The symbol was not found at runtime on MacOS.

That's odd, because on 3.0 it's already there as a non-static function: https://github.com/ruby/ruby/blob/v3_0_0/process.c#L622-L634
And I see it defined with nm $(which ruby) | grep rb_process_status_new or nm ~/.rubies/ruby-3.0.1/lib/libruby.so | grep rb_process_status_new on Linux.

MacOS differentiates external vs. non-external symbols, eventmachine/eventmachine#948 (comment)

I have already written my thoughts about this problem, and have suggested a solution that avoids this by changing the fiber scheduler #process_wait API to return [pid, status] instead of an instance of Process::Status. This is then converted internally to a Process::Status. Seems much simpler to me.

API to return [pid, status]

It's insufficient since errno is missing. This is why Process::Status.wait is good, because it hides the implementation.

It's insufficient since errno is missing.

Then why not add it as a third array member to the return value: [pid, status, errno]?

FWIW the errno is not exposed in the Process::Status API (at least as I could tell). When calling Process.wait (which is I guess what most people use), any error (e.g. a bad pid) will raise an exception.

@ciconia at some point you need to choose an interface. Whether you return a tuple, or the actual object, seems relatively unimportant (except that it avoids extra allocations). While I appreciate your dedication to this topic, I'm not sure what we have to gain by discussing this particular detail of the interface since it's largely set in stone now.

FWIW the errno is not exposed in the Process::Status API

Nope, and it never has been, but it's still a critical detail that needs to transit the scheduler boundary in order to be handled correctly by Process.wait and similar methods.

Please keep in mind that I did not build these interfaces, I did not design them, and I'm largely unable to change them. I look for the path of least resistance for fiber scheduler interface, and at the time, given the constraints I faced, that was the decision I made. Sometimes making a decision and rolling with it is more important than the actual specific design details, since they are all largely equivalent and or carry similar pros/cons which are themselves significantly outweighed by the benefit of having something that works. So while your criticism might be valid, I'm not sure how it helps.

Regarding immutability, I've opened this discussion https://bugs.ruby-lang.org/issues/18035 - whatever decision we make, that we agree to apply consistently across all of stdlib, I will happily implement here.

I'm not sure what we have to gain by discussing this particular detail of the interface since it's largely set in stone now.

From what I can see the scheduler interface is still being actively changed, so why is that one OK but not the other?

Please keep in mind that I did not build these interfaces, I did not design them, and I'm largely unable to change them. I look for the path of least resistance for fiber scheduler interface, and at the time, given the constraints I faced, that was the decision I made. Sometimes making a decision and rolling with it is more important than the actual specific design details, since they are all largely equivalent and or carry similar pros/cons which are themselves significantly outweighed by the benefit of having something that works. So while your criticism might be valid, I'm not sure how it helps.

I appreciate your point of view, but then again since fiber scheduling is still not "done" (as in, finalized API and functionality-I see you adding new stuff all the time-which is great!) I don't really see why we need to look at it as "set in stone". The present discussion (as well as others such as #4621) shows some of the design details are not "largely equivalent" and have consequences which might add complexity or require reworking significant amounts of code. I think the solution I proposed is not only valid but also much easier to implement (that is not "equivalent" as you maintain) even if it demands a backward-incompatible change to the fiber scheduler interface.

I do wish there was some document describing the design of fiber scheduling in terms of API and of how it relates to the rest of the Ruby runtime, and which could be discussed and iterated on before committing to certain decisions. I'd really like to contribute from my own experience, but I feel like the design decisions are made by a single person - albeit a very capable one such as yourself - without any public discussion or public weighing of alternatives. Such a public design process (and it's not too late to start one!) might have helped avoid some of the effort we're seeing, now that we finally get into the details.

why is that one OK but not the other?

Some parts are part of the public interface and other parts are experimental. The public interface is documented in fiber.md. https://github.com/ruby/ruby/blob/ruby_3_0/doc/fiber.md#interface

I don't really see why we need to look at it as "set in stone"

process_wait is documented as part of the public interface. https://github.com/ruby/ruby/blob/ruby_3_0/doc/fiber.md#interface

I think the solution I proposed is not only valid but also much easier to implement.

I don't think so. You will have to implement wait for POSIX, Windows, and potentially other edge cases. Process::Status.wait hides all the implementation, that's my personal preference. However, I do respect that some scheduler implementations (e.g. EventMachine in this case) already have the status by the time it needs to create a status object, and that's what this PR is trying to support.

Feel free to review

Such a public design process (and it's not too late to start one!) might have helped avoid some of the effort we're seeing

Almost all the discussions are happening on bugs.ruby-lang.org. https://bugs.ruby-lang.org/issues/17369 - please feel free to get involved in https://bugs.ruby-lang.org/issues/18020 and https://bugs.ruby-lang.org/issues/18035

I will add this clarification to fiber.md:

The scheduler interface is designed to be a un-opinionated light-weight layer
between user code and blocking operations. The scheduler hooks should avoid
translating or converting arguments or return values. Ideally, the exact same
arguments from the user code are provided directly to the scheduler hook with
no changes.

In this case, the blocking operation is Process.wait.

@eregon at #4563 (comment)

And I see it defined with nm $(which ruby) | grep rb_process_status_new or nm ~/.rubies/ruby-3.0.1/lib/libruby.so | grep rb_process_status_new on Linux.

You should add --extern-only --defined-only to nm.
There are 3 levels, visible inside each compile unit, inside libruby, and publicly.
The patch to include/ruby/internal/intern/process.h will make this function publicly visible.

@nobu Thanks for the explanation!