Skip to content

Commit

Permalink
Fix Process::Status for unknown signals (#15280)
Browse files Browse the repository at this point in the history
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).
  • Loading branch information
straight-shoota authored Dec 17, 2024
1 parent 2f3e07d commit 1df4b23
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 4 deletions.
8 changes: 8 additions & 0 deletions spec/std/process/status_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,9 @@ describe Process::Status do
Process::Status.new(Signal::INT.value).exit_signal.should eq Signal::INT
last_signal = Signal.values[-1]
Process::Status.new(last_signal.value).exit_signal.should eq last_signal

unknown_signal = Signal.new(126)
Process::Status.new(unknown_signal.value).exit_signal.should eq unknown_signal
end

it "#normal_exit? with signal code" do
Expand Down Expand Up @@ -249,6 +252,8 @@ describe Process::Status do
assert_prints Process::Status.new(Signal::HUP.value).to_s, "HUP"
last_signal = Signal.values[-1]
assert_prints Process::Status.new(last_signal.value).to_s, last_signal.to_s

assert_prints Process::Status.new(Signal.new(126).value).to_s, "Signal[126]"
end
{% end %}
end
Expand All @@ -275,6 +280,9 @@ describe Process::Status do
assert_prints Process::Status.new(Signal::HUP.value).inspect, "Process::Status[Signal::HUP]"
last_signal = Signal.values[-1]
assert_prints Process::Status.new(last_signal.value).inspect, "Process::Status[#{last_signal.inspect}]"

unknown_signal = Signal.new(126)
assert_prints Process::Status.new(unknown_signal.value).inspect, "Process::Status[Signal[126]]"
end
{% end %}
end
Expand Down
3 changes: 2 additions & 1 deletion src/enum.cr
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,8 @@ abstract struct Enum
end
end

private def member_name
# :nodoc:
def member_name : String?
# Can't use `case` here because case with duplicate values do
# not compile, but enums can have duplicates (such as `enum Foo; FOO = 1; BAR = 1; end`).
{% for member in @type.constants %}
Expand Down
12 changes: 9 additions & 3 deletions src/process/status.cr
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ class Process::Status
# which also works on Windows.
def exit_signal : Signal
{% if flag?(:unix) && !flag?(:wasm32) %}
Signal.from_value(signal_code)
Signal.new(signal_code)
{% else %}
raise NotImplementedError.new("Process::Status#exit_signal")
{% end %}
Expand Down Expand Up @@ -298,7 +298,12 @@ class Process::Status
if normal_exit?
io << exit_code
else
io << exit_signal
signal = exit_signal
if name = signal.member_name
io << name
else
signal.inspect(io)
end
end
{% end %}
end
Expand All @@ -314,7 +319,8 @@ class Process::Status
if normal_exit?
exit_code.to_s
else
exit_signal.to_s
signal = exit_signal
signal.member_name || signal.inspect
end
{% end %}
end
Expand Down

0 comments on commit 1df4b23

Please sign in to comment.