You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This is another issue about portability of Process::Status, sparked by the discussion around #8381.
Process::Status#exit_signal returns a Signal instance if the process was terminated by a signal. Otherwise it raises.
There are a couple of problems with the current behaviour:
Error handling
The raised exception is inconsistent and unhelpful: On Windows, this method unconditionally raises NotImplementedError. On Unix it raises a plain exception Exception with message Unknown enum Signal value: 0 (from Enum.from_value).
This should rather be a unified error saying that the process status does not have an exit signal (similar to #15241 for #exit_code).
However, raising is a very poor way to indicate the absence of a value. And this is by far not an error condition. Typically, most processes exit normaly and not with an exit signal. On Windows no process exits with a signal. So this is far from exceptional. Not having an exit status is just a normal condition. I believe that should better be expressed with a nil return type than raising an exception.
Simply changing the return type of #exit_signal is not possible for backward compatibility. So instead I'd suggest to introduce a new method #exit_signal? with the return type Signal | Nil (similar to #exit_code? from #15247).
We could consider deprecating the raising variants #exit_signal and #exit_code. I think that would make sense as it's not very intutitive that they are quite likely to raise in a situation that's not really exceptional.
If we want to keep raising variants, I'd suggest they should be named #exit_signal! and #exit_code!. The exclamation mark makes it more explicit that these methods raise as a normal error condition. That said, there's not too much benefit over the nil-returning variants. They essentially just contribute a nice error message in cases where you expect the value to be present but it isn't. But that could be good value, actually.
Valid range
Using Signal.from_value limits the result to named members of the enum Signal. That means it only accepts signals that are explicitly defined in the Crystal bindings. While we generally strive for completeness, there's no guarantee that the operating system might indicate a signal that is not in the bindings. And in fact there are no mappings for real-time signals (SIGRTMIN..SIGRTMAX), for example (not sure if they're particularly relevant as exit signals, but it shows incompleteness).
This mechanism should be fault tolerant and be able to represent any signal value, regardless of whether it's mapped in the Crystal bindings or not. If it's missing we cannot associate a name, but we know it's a signal and can express that in the type Signal (enums can take any value, not just those of named members).
This can be achieved rather trivially by changing Signal.from_value(signal_code) to Signal.new(signal_code), making the type conversion unconditional.
The text was updated successfully, but these errors were encountered:
This is another issue about portability of
Process::Status
, sparked by the discussion around #8381.Process::Status#exit_signal
returns aSignal
instance if the process was terminated by a signal. Otherwise it raises.There are a couple of problems with the current behaviour:
Error handling
The raised exception is inconsistent and unhelpful: On Windows, this method unconditionally raises
NotImplementedError
. On Unix it raises a plain exceptionException
with messageUnknown enum Signal value: 0
(fromEnum.from_value
).This should rather be a unified error saying that the process status does not have an exit signal (similar to #15241 for
#exit_code
).However, raising is a very poor way to indicate the absence of a value. And this is by far not an error condition. Typically, most processes exit normaly and not with an exit signal. On Windows no process exits with a signal. So this is far from exceptional. Not having an exit status is just a normal condition. I believe that should better be expressed with a
nil
return type than raising an exception.Simply changing the return type of
#exit_signal
is not possible for backward compatibility. So instead I'd suggest to introduce a new method#exit_signal?
with the return typeSignal | Nil
(similar to#exit_code?
from #15247).We could consider deprecating the raising variants
#exit_signal
and#exit_code
. I think that would make sense as it's not very intutitive that they are quite likely to raise in a situation that's not really exceptional.If we want to keep raising variants, I'd suggest they should be named
#exit_signal!
and#exit_code!
. The exclamation mark makes it more explicit that these methods raise as a normal error condition. That said, there's not too much benefit over the nil-returning variants. They essentially just contribute a nice error message in cases where you expect the value to be present but it isn't. But that could be good value, actually.Valid range
Using
Signal.from_value
limits the result to named members of the enumSignal
. That means it only accepts signals that are explicitly defined in the Crystal bindings. While we generally strive for completeness, there's no guarantee that the operating system might indicate a signal that is not in the bindings. And in fact there are no mappings for real-time signals (SIGRTMIN
..SIGRTMAX
), for example (not sure if they're particularly relevant as exit signals, but it shows incompleteness).This mechanism should be fault tolerant and be able to represent any signal value, regardless of whether it's mapped in the Crystal bindings or not. If it's missing we cannot associate a name, but we know it's a signal and can express that in the type
Signal
(enums can take any value, not just those of named members).This can be achieved rather trivially by changing
Signal.from_value(signal_code)
toSignal.new(signal_code)
, making the type conversion unconditional.The text was updated successfully, but these errors were encountered: