Skip to content

Commit 153e965

Browse files
Add timeout protection for version checking operations
Extract hardcoded timeout value to VERSION_CHECK_TIMEOUT constant for better maintainability and testability. Co-authored-by: Abanoub Ghadban <AbanoubGhadban@users.noreply.github.com>
1 parent 1feef05 commit 153e965

File tree

2 files changed

+13
-10
lines changed

2 files changed

+13
-10
lines changed

lib/react_on_rails/dev/process_manager.rb

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,9 @@
55
module ReactOnRails
66
module Dev
77
class ProcessManager
8+
# Timeout for version check operations to prevent hanging
9+
VERSION_CHECK_TIMEOUT = 5
10+
811
class << self
912
# Check if a process is available and usable in the current execution context
1013
# This accounts for bundler context where system commands might be intercepted
@@ -49,7 +52,7 @@ def installed_in_current_context?(process)
4952
# Use system() because that's how we'll actually call it later
5053
version_flags_for(process).any? do |flag|
5154
# Add timeout to prevent hanging on version checks
52-
Timeout.timeout(5) do
55+
Timeout.timeout(VERSION_CHECK_TIMEOUT) do
5356
system(process, flag, out: File::NULL, err: File::NULL)
5457
end
5558
end
@@ -103,7 +106,7 @@ def process_available_in_system?(process)
103106
# Try version flags to check if process exists outside bundler context
104107
version_flags_for(process).any? do |flag|
105108
# Add timeout to prevent hanging on version checks
106-
Timeout.timeout(5) do
109+
Timeout.timeout(VERSION_CHECK_TIMEOUT) do
107110
system(process, flag, out: File::NULL, err: File::NULL)
108111
end
109112
end

spec/react_on_rails/dev/process_manager_spec.rb

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -19,19 +19,19 @@
1919

2020
describe ".installed?" do
2121
it "returns true when process is available in current context" do
22-
expect(Timeout).to receive(:timeout).with(5).and_yield
22+
expect(Timeout).to receive(:timeout).with(described_class::VERSION_CHECK_TIMEOUT).and_yield
2323
expect_any_instance_of(Kernel).to receive(:system)
2424
.with("overmind", "--version", out: File::NULL, err: File::NULL).and_return(true)
2525
expect(described_class).to be_installed("overmind")
2626
end
2727

2828
it "returns false when process is not available in current context" do
29-
expect(Timeout).to receive(:timeout).with(5).and_raise(Errno::ENOENT)
29+
expect(Timeout).to receive(:timeout).with(described_class::VERSION_CHECK_TIMEOUT).and_raise(Errno::ENOENT)
3030
expect(described_class.installed?("nonexistent")).to be false
3131
end
3232

3333
it "returns false when all version flags fail" do
34-
expect(Timeout).to receive(:timeout).with(5).exactly(3).times.and_yield
34+
expect(Timeout).to receive(:timeout).with(described_class::VERSION_CHECK_TIMEOUT).exactly(3).times.and_yield
3535
expect_any_instance_of(Kernel).to receive(:system)
3636
.with("failing_process", "--version", out: File::NULL, err: File::NULL).and_return(false)
3737
expect_any_instance_of(Kernel).to receive(:system)
@@ -42,7 +42,7 @@
4242
end
4343

4444
it "returns true when second version flag succeeds" do
45-
expect(Timeout).to receive(:timeout).with(5).twice.and_yield
45+
expect(Timeout).to receive(:timeout).with(described_class::VERSION_CHECK_TIMEOUT).twice.and_yield
4646
expect_any_instance_of(Kernel).to receive(:system)
4747
.with("foreman", "--version", out: File::NULL, err: File::NULL).and_return(false)
4848
expect_any_instance_of(Kernel).to receive(:system)
@@ -51,7 +51,7 @@
5151
end
5252

5353
it "returns false when version check times out" do
54-
expect(Timeout).to receive(:timeout).with(5).and_raise(Timeout::Error)
54+
expect(Timeout).to receive(:timeout).with(described_class::VERSION_CHECK_TIMEOUT).and_raise(Timeout::Error)
5555
expect(described_class.installed?("hanging_process")).to be false
5656
end
5757
end
@@ -158,7 +158,7 @@
158158
describe ".process_available_in_system?" do
159159
it "checks process availability outside bundle context with version flags" do
160160
expect(described_class).to receive(:with_unbundled_context).and_yield
161-
expect(Timeout).to receive(:timeout).with(5).and_yield
161+
expect(Timeout).to receive(:timeout).with(described_class::VERSION_CHECK_TIMEOUT).and_yield
162162
expect_any_instance_of(Kernel).to receive(:system)
163163
.with("foreman", "--version", out: File::NULL, err: File::NULL).and_return(true)
164164

@@ -173,7 +173,7 @@
173173

174174
it "tries multiple version flags before failing" do
175175
expect(described_class).to receive(:with_unbundled_context).and_yield
176-
expect(Timeout).to receive(:timeout).with(5).twice.and_yield
176+
expect(Timeout).to receive(:timeout).with(described_class::VERSION_CHECK_TIMEOUT).twice.and_yield
177177
expect_any_instance_of(Kernel).to receive(:system)
178178
.with("foreman", "--version", out: File::NULL, err: File::NULL).and_return(false)
179179
expect_any_instance_of(Kernel).to receive(:system)
@@ -184,7 +184,7 @@
184184

185185
it "returns false when version check times out in system context" do
186186
expect(described_class).to receive(:with_unbundled_context).and_yield
187-
expect(Timeout).to receive(:timeout).with(5).and_raise(Timeout::Error)
187+
expect(Timeout).to receive(:timeout).with(described_class::VERSION_CHECK_TIMEOUT).and_raise(Timeout::Error)
188188

189189
expect(described_class.send(:process_available_in_system?, "hanging_process")).to be false
190190
end

0 commit comments

Comments
 (0)