Deprecate Process::Status#exit_status

[jwoertink]

I originally ran in to some confusion when using exit_status and getting a 0 even though the program was raising an exception. It turned out that exit_code was the correct method here.

As per @RX14 comment: #8381 (comment) I have deprecated the exit_status method in favor of just using exit_code.

Fixes #8381

[refi64]

IMO this is still useful in a platform-specific context and shouldn't be removed, maybe just renamed to platform_exit_status or similar?

…

On Fri, Jan 3, 2020, 7:33 PM Sijawusz Pur Rahnama ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/process/status.cr <#8647 (comment)>: > @@ -25,6 +22,13 @@ class Process::Status Signal.from_value(signal_code) end + # Platform-specific exit status code, which usually contains either the exit code or a termination signal. + # The other `Process::Status` methods extract the values from `exit_status`. + @[Deprecated("Use `Process::Status#exit_code`")] IIRC it should — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#8647?email_source=notifications&email_token=AAM4YSMHQCMEQJDTKJNADUDQ37RP3A5CNFSM4KCTCAZKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCQVIFNA#discussion_r363007780>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAM4YSNHA534RLC5TKEMZ3TQ37RP3ANCNFSM4KCTCAZA> .

[jwoertink]

@refi64 In the other thread there was a mention of possibly renaming it. I'm cool with that option too. I'll wait for some more feedback before I change anything though.

[RX14]

I think it can be removed, it's easy enough to bitshift it back if required.

[refi64]

That's not really the same. Since exit_code is undefined if the process didn't exit normally, you'd have to have a chain of ifs tested if it was exited, signaled, or stopped, and then reconstructing the original exit status.

…

On Sat, Jan 4, 2020, 6:26 PM Stephanie Hobbs ***@***.***> wrote: I think it can be removed, it's easy enough to bitshift it back if required. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8647?email_source=notifications&email_token=AAM4YSNZPVJRZGUMSVGGI7LQ4ESK3A5CNFSM4KCTCAZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIDDLYA#issuecomment-570832352>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAM4YSM27TVUD4RZDAJEMO3Q4ESK3ANCNFSM4KCTCAZA> .

[straight-shoota]

Simple question: How's this implemented in other languages? We don't need to reinvent such basic things, just look what others are doing and pick the best one.

[jwoertink]

I'm down for any suggestions here. I think the main thing is that it's clear what one does versus another. It almost feels like these two method names are reversed. I tried looking at a few other languages, but trying to decipher low level process stuff is a little over my head.

[jan-zajic]

In Go lang, ExitCode() returns the exit code of the exited process, or -1 if the process hasn't exited or was terminated by a signal.

In Rust, ExitStatus has method code, that Returns the exit code of the process, if any. On Unix, this will return None if the process was terminated by a signal;

In Nim, Process method peekExitCode Return -1 if the process is still running. Otherwise the process' exit code. On posix, if the process has exited because of a signal, 128 + signal number will be returned.

That +128 is similiar what all Unix shells do. for example in Bash documented here

You can find similar note in Java native sources (Solaris implementation):

/* The child exited because of a signal.
The best value to return is 0x80 + signal number,
because that is what all Unix shells do, and because
it allows callers to distinguish between process exit and
process death by signal.
Unfortunately, the historical behavior on Solaris is to return
the signal number, and we preserve this for compatibility. */

But since we have wrapping Process::Status class, we can easily deprecate exit_status method, as proposed, so +1 for that. Any other informations encoded in status can be easily accessed by other methods like exit_signal and normal_exit?. Is for discussion what to return from exit_code method if exited abnormally, but I think it is subject for another PR?

[jwoertink]

Thanks for looking up that info @jan-zajic

[straight-shoota]

We should probably follow Go and Rust in only returning an exit code if there is actually one and not tread a signal indicator as an exit code. This may be fine for more platform-specific implementations, but for a portable language it feels wrong when a child doesn't exit but the process status reports an exit code.

Go's implementation also provides access to system-specific information, such as extracting the singal number or whether the process was core dumped. The API is hidden in syscall.WaitStatus which can be accessed but isn't documented in the public API. I'm not sure about the reason for this as it appears that the same API works on all platforms (on windows it simply always return null status). So it should actually be fine to have this publicly documented.
Rust on the other hand provides more detailed information in a system-specific package.
That's probably the best solution. We are going to have a shard for system-specific implementations such as fork anyway, so it would fit there very well. This would also mean to remove most of the method in Process::Status (#exit_signal, #normal_exit?, #signal_exit?).

Is for discussion what to return from exit_code method if exited abnormally, but I think it is subject for another PR?

I'd prefer to combine deprecation of #exit_status and possible changes to #exit_code in one PR because they belong together and should produce a breaking change only once.

[oprypin]

I think it's very clear that we shouldn't expose exit_status, I don't know any other language that does.
What does worry me, though, is that a process can exit with a signal but have exit_code == 0. I don't know any other language that does this either. Usually it causes the exit code to be -signal or 128 + signal.

So, just due to this, it's not 100% obvious that this is a good change, as it might direct people towards checking exit_code == 0, which is worse than exit_status == 0.

[oprypin]

If I was making this decision on my own, I'd merge this PR immediately and then also make exit_code return -signal_code if signal_exit?

[refi64]

I've had to rebuild exit status values before because a language doesn't expose it, it's a pretty painful experience and I don't really see any loss from having e.g. platform_exit_status.

[oprypin]

Having explored the full variety of exit statuses and how poorly they are covered by the convenience methods, now I'm not so sure that we are ready to deprecate this. In addition to what @refi64 said.

[straight-shoota]

issue: Status#exit_code calls #exit_status internally. We should expand that to @exit_status.to_i32!.
(I could not add this comment in the affected line directly because it's not part of the diff.)

Stale

[oprypin]

What about the footgun that I had mentioned?

If people switch from checking exit_status == 0 to exit_code == 0, this will now ignore any signal exit of the process

[straight-shoota]

@oprypin That's a good point. I suppose this could be a potential foot gun.

Legitimate use cases appear to be relatively rare though. And I'd expect people who write code for explicitly handling the platform-specific exit status to take the specifics into account. For this we should probably still provide acces to the platform-specific value, but it could be a separate feature addition (as per #8381 (comment)).

So I agree a relevant foot gun could be an uninformed transformation from exit_status == 0 (or an equivalent) to exit_code == 0. This could be quite common when translating code from other languages.

The fact that exit_code returns 0 on a signal exit is quite problematic and a footgun even without considering the deprecation of exit_status. There simply is no exit code when the process did not exit normally.
So I think the solution would be for exit_code to raise on signal exit.

[straight-shoota]

This PR is generally approved but we'll hold merging it until #15228 (and possibly other related issues) are resolved to concentrate the changes.

Resolved

[straight-shoota]

We've progressed a bit more. #exit_code now raises on abnormal exit since #15241 and the definition of normal (and thus abnormal) has been clarified in #15255. And #15247 added #exit_code? as a non-raising alternative.

So far we have not discussed exposing the platform-specific exit status in a different manner.
The related, platform-specific methods #signal_exit? and #exit_signal are also questionable but shouldn't be a blocker for this (#15271).