Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Process::Status#exit_code returns 0 on signal exit #15228

Closed
straight-shoota opened this issue Nov 25, 2024 · 3 comments · Fixed by #15241
Closed

Process::Status#exit_code returns 0 on signal exit #15228

straight-shoota opened this issue Nov 25, 2024 · 3 comments · Fixed by #15241

Comments

@straight-shoota
Copy link
Member

This issue is derived from #8647 (review) but I figure it deserves a separate discussion from that PR and #8381 because it's a different issue.

Process::Status#exit_code is supposed to return the exit code of a process. An exit code only exists when the process had a normal exit, though. On a signal exit, the value of exit_code would be undefined.
However, currently, it returns 0 on a signal exit which is highly irritating.

This could clearly mislead users who assume that status.exit_code == 0 implies a succesful exit. Of course there is a dedicated method Process::Status#success? for that and it operates correctly, but that's easy to overlook.

status = Process::Status.new(Signal::INT.value)
status.exit_reason  # => Process::ExitReason::Interrupted
status.normal_exit? # => false
status.exit_code    # => 0
status.success?     # => false

It seems very wrong to report an exit code of 0 when the process did not even exit normally.
And status.success? should be equivalent to status.exit_code.zero?.

The documentation does not specify any behaviour on a signal exit, it merely states:

If normal_exit? is true, returns the exit code of the process.

I believe the best solution would be for exit_code to raise when the process did not exit normally.
It might technically be preferable to return nil in this case, but that would break the method's signature.
Raising does not it. It only affects behaviour. Still in a breaking way, but the current behaviour is clearly wrong. Code that depends on exit_code == 0 on a signal exit depends on a bug.

We could consider introducing #exit_code? as an alternative which does not raise but retrun nil on signal exit.

@oprypin
Copy link
Member

oprypin commented Nov 25, 2024

Yes. I just think that ideally a solution to this would happen in the same release as #8647

This is for better awareness (a user might discover both changes at once when looking at the deprecation message for one of them) and to avoid recommending more usages of #exit_code before it's fixed.

@ysbaddaden
Copy link
Contributor

ysbaddaden commented Nov 26, 2024

Raising an exception instead of undefined/invalid behavior and introducing a nilable version is 👍

@straight-shoota
Copy link
Member Author

The question about what consitutes a "normal exit" (#15231) and the quest for a portable definition also relates to #exit_code:
Should statuses of abnormal exits on Windows (everything that's not ExitReason::Normal) still return an exit code such as LibC::STATUS_CONTROL_C_EXIT?
These status codes indicate that the process did not exit normally and from that I think it should follow that it does not have a exit code.
The behaviour would align well on Windows and Unix that #exit_code only reports the actual exit codes of the procsess, not OS-specific implementation details for communicating abnormal status.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants