Exit status consistency issue

[jwoertink]

When you raise in a file, then check your exit status from the terminal, you'll see 1, but it seems if that raise comes from within a Process, then you don't get that status code:

# test.cr
raise "test"

# run.cr
exit Process.run(
  "crystal run test.cr",
  shell: true,
  input: STDIN,
  output: STDOUT,
  error: STDERR).exit_status

# $ crystal test.cr; echo $? #=> 1
# $ crystal run.cr; echo $? #=> 0

I expected running crystal run.cr would return a exit status code of 1 but got 0.

[12:12PM] sandbox$ crystal -v
Crystal 0.31.1 (2019-09-30)

LLVM: 6.0.1
Default target: x86_64-apple-macosx
macOS 10.15

ref: https://gitter.im/crystal-lang/crystal?at=5db34583fb4dab784a05c80d

[Blacksmoke16]

You probably want to use exit_code not exit_status.

[straight-shoota]

@Blacksmocke16 There should not be a difference between exit_code and exit_status when the value is 0.

[jwoertink]

exit_code actually does give me what I want. I'm not sure what the difference is now

[RX14]

I think we should remove or rename exit_status. It's nothing more than a platform-specific encoding of the exit code + signal number.

[jwoertink]

@RX14 Would a good option here be to deprecate exit_status and just say to use exit_code and maybe mention it may be removed later?

[RX14]

yep

[oprypin]

For your enjoyment,
I collected the data on what the macros actually return; what patterns they have.
Naming below based on man wait(2).

First column is the status code in hexadecimal;
* means earlier bytes don't matter;
XX means it can be any byte but the result depends on it.

*XX00: EXITED, EXITSTATUS=XX

  *01: SIGNALED, TERMSIG=1
  ...                    ...
  *7E: SIGNALED, TERMSIG=126

*XX7F: STOPPED, STOPSIG=XX

*XX80: EXITED, EXITSTATUS=XX, COREDUMP  

  *81: SIGNALED, TERMSIG=1, COREDUMP
  ...                    ...
  *FE: SIGNALED, TERMSIG=126, COREDUMP

  *FF: COREDUMP

 FFFF: CONTINUED

(note no wildcard on the last one)

The striked out lines are invalid states but I'm posting what the macros happen to produce anyway.

The program to produce this

#include <sys/wait.h>
#include <stdio.h>

int main() {
    const int exited = 1, signaled = 2, stopped = 4, continued = 8, coredump = 16;

    int prev_kind = 0;
    int prev_value = 0;
    for (int n = 0; n < 0x100000; ++n) {
        int kind = 0;
        if (WIFEXITED(n))
            kind |= exited;
        if (WIFSIGNALED(n))
            kind |= signaled;
        if (WIFSTOPPED(n))
            kind |= stopped;
        if (WIFCONTINUED(n))
            kind |= continued;
        if (WCOREDUMP(n))
            kind |= coredump;
        if (kind != prev_kind) {
            printf("\n");
        }
        printf("%4x", n);
        int value = -1;
        if (kind & exited)
            printf("  exit %3d", value = WEXITSTATUS(n));
        if (kind & signaled)
            printf("  signal %3d", value = WTERMSIG(n));
        if (kind & stopped)
            printf("  stop %3d", value = WSTOPSIG(n));
        if (kind & continued)
            printf("  continued");
        if (kind & coredump)
            printf("  coredump");
        printf("                   ");
        if (kind == prev_kind && value == prev_value + 1)
            printf("\r");
        else
            printf("\n");
        prev_kind = kind;
        prev_value = value;
    }
}

So in terms of what the stdlib allows at the moment (or rather doesn't expose):

• if neither normal_exit? nor signal_exit? then it could be STOPPED (with any STOPSIG) or CONTINUED, can't tell.
• if signal_exit? then it could be either COREDUMP or not, can't tell.
• both exit_code and signal_code are allowed to be called when their value is nonsense.
  • specifically, if one calls exit_code when it's a signal_exit?, it could be anything (but probably 0, which is horrible)

[stronny]

@oprypin wanted me to repost this idea from gitter, so there you go:

abstract struct Process::Status
  struct Exited < Process::Status
    def status : Int8
  end
  struct Stopped < Process::Status
    def stop_signal : Int8
  end
  struct Continued < Process::Status
  end
  struct Aborted < Process::Status # Signaled or Signalled look weird
    def signal : Int8
    def core_dumped? : Bool
  end
end

[straight-shoota]

Having separate types seems way overkill to me. I'd rather keep it simple and don't implement every single POSIX feature. The vast majority of use cases only asking for success?, and maybe a few are interested in the exit status.

[stronny]

I see the virtue in simplicity, however the solution needs to distinguish between a valid exit status and its absense altogether. The doc says that:

def exit_code

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

And afaiu it's user's job to remember to always check for normal_exit before exit_code, lest the result would silently be invalid.

Moreover removing exit_status would make precise handling impossible.

[RX14]

I'd like to keep it one type too.

[straight-shoota]

I've already described my preferred solution in #8647 (comment)

If the process exited with a status code, exit_code returns that code, otherwise nil. Then we don't need normal_exit? and signal_exit?, because that's handled by the nil return value.

Moreover removing exit_status would make precise handling impossible.

We can make the platform specific raw exit code available. Either as a special property or through a platform specific shard which could also include other advanced POSIX-specific features.

[stronny]

There is also an option to do what bash does: https://www.gnu.org/software/bash/manual/html_node/Exit-Status.html

[oprypin]

I would much prefer to do what Python does, as that doesn't cannibalize the number space of actual exit codes 128-255.

So, exit_code would not be nillable, just have a negative number instead for signals. Backwards-compatible and more convenient.

[stronny]

On the one hand that makes sense, but on the other if what you need is to pass that code higher up, using it as an argument to your own exit you will have to do additional motions, because shell will eat 126+ anyway, and you can't just exit with a negative value.

[oprypin]

I don't think passing signals is something you want to do

[stronny]

That's what shell does:

1> $ (sleep 600)
2> $ killall sleep
1> Terminated
1> $ echo $?
1> 143

[straight-shoota]

Some of the inconsistencies regarding exist statuses were fixed with #exit_reason in #13052.

I think the only remaining issue is that #exit_status is platform-specific and rarely useful. Most of the time, you actually want to use #exit_code. It's hard to tell them apart because the names are similar and don't immediately communicate their different behaviours.
So I think we should remove (first deprecate of course) #exit_status, as porposed in #8647.
We could consider introducing a replacement which makes the platform-specific nature explicit. Maybe #system_exit_status or #platform_exit_status?
I'd like to see an actual use case for this (ping @refi64).

[straight-shoota]

It probably makes sense to expose the platform-specific value in an explicit and documented manner. It is already available through direct access on the instance variable (e.g. status.@exit_status) anyway. A method would make it part of the documented API.

It seems like a good idea to name the method #system_exit_status.

It's a bit tricky to decide what its return type should be, though. The native values are Int32 on Unix and UInt32 on Windows. At least the size is correct, but we still have different signendness.
#exit_status returns Int32, converting the unsigned Windows values to signed ones. That turns abnormal exit codes (which are relatively big numbers) into negative numbers, which is certainly not very ideal.

I think we have two options:

1. Standardize on a portable type. UInt32 seems like a reasonable choice. According to Oprypin's analysis (Exit status consistency issue #8381 (comment)) the status code on Unix systems uses only 2 significant bytes and is never negative. So UInt32 should be able to represent all relevant values.
2. Let the specific return type be platform-dependent and document it as a union Int32 | UInt32. Potentially even explicitly cast the return value with .as(Int32 | UInt32) so calling code is forced to handle both.

[ysbaddaden]

I'd go for 3. the return type is platform dependent. It's a platform specific method that returns a platform specific value after all.

[oprypin]

@ysbaddaden even if that makes the most sense, it goes directly against how all other platform-dependent APIs have been defined so far

[straight-shoota]

@ysbaddaden Isn't that option 2 without the explicit cast? (I suppose the return type restriction could be optional as well, but what would be the point of that?)

Usually types should be identical across platforms in order to have a stable and portable API. The idea is that it should allow code that works on one platform work on any other platform as well. But this is hardly possible with the platform-specific exit status. You need platform-specific code to interpret it.

On the other hand, option 1. seems like a relatively effortless way to coerce the same type on all platforms, which I figure should be fine?

[ysbaddaden]

@straight-shoota yeah, 3. is 2. without the explicit cast so it's fully system dependent (even if we add another target someday).

I'm not sure about 1. because it would cast u32 to i32 and then we can't compare with constants without casting back to u32, can we?

[straight-shoota]

My proposal was to cast from i32 to u32 because the range of valid/significant values is positive on Windows and Unix and can be greater than Int32::MAX on Windows.

So direct comparison with constants should work trivially on Windows because UInt32 is the native type.

On Unix systems you don't compare to constants directly anyway, you need to apply a bit mask.
I believe that should generally still work though. As Oleh's comment shows, significant values on Unix systems appear to use only 2 bytes, so behaviour should be identical between Int32 and UInt32.

[ysbaddaden]

My Bad. I thought casting from u32 to i32 🤦

[straight-shoota]

Not sure where that leaves us now with regards to the expected return type 🤔
@ysbaddaden, @oprypin any preference?

I suppose UInt32 would be the simpler solution

[ysbaddaden]

I suppose it's fine.