Stop using Ruby's dangerous Timeout API

laserlemon · Apr 23, 2024 · fe535e4 · fe535e4
1 parent 1a08e84
commit fe535e4
Show file tree

Hide file tree

Showing 7 changed files with 97 additions and 138 deletions.
diff --git a/.rubocop.yml b/.rubocop.yml
@@ -21,6 +21,8 @@ Metrics:
 Naming/RescuedExceptionsVariableName:
   Enabled: false
 
+RSpec/BeEq:
+  Enabled: false
 RSpec/DescribeClass:
   Enabled: false
 RSpec/DescribedClass:

diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -16,6 +16,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 - Added RuboCop for consistent code style
 - Move CI from Travis to GitHub Actions
+- Stop using Ruby's dangerous `Timeout` API.
+  Only evaluate the timeout condition after successfully calling the matcher's block, never mid-call.
 
 ### Removed
 
@@ -24,6 +26,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Support for RSpec versions 3.0 through 3.3
 - RSpec::Wait.version (in favor of RSpec::Wait::VERSION)
 - Passing an argument to wait_for or wait.for (must pass a block)
+- RSpec::Wait::TimeoutError in favor of RSpec failure
 
 ## [0.0.9] - 2016-07-11
 

diff --git a/lib/rspec/wait.rb b/lib/rspec/wait.rb
@@ -1,7 +1,6 @@
 # frozen_string_literal: true
 
 require "rspec"
-require "rspec/wait/error"
 require "rspec/wait/handler"
 require "rspec/wait/proxy"
 require "rspec/wait/target"

diff --git a/lib/rspec/wait/error.rb b/lib/rspec/wait/error.rb
diff --git a/lib/rspec/wait/handler.rb b/lib/rspec/wait/handler.rb
@@ -11,16 +11,17 @@ module Wait
     # matcher passes or the configured timeout elapses.
     module Handler
       def handle_matcher(target, *args, &block)
-        failure = nil
+        start_time = Process.clock_gettime(Process::CLOCK_MONOTONIC)
 
-        Timeout.timeout(RSpec.configuration.wait_timeout) do
+        begin
           super(target.call, *args, &block)
-        rescue RSpec::Expectations::ExpectationNotMetError => failure
+        rescue RSpec::Expectations::ExpectationNotMetError
+          elapsed_time = Process.clock_gettime(Process::CLOCK_MONOTONIC) - start_time
+          raise if elapsed_time > RSpec.configuration.wait_timeout
+
           sleep RSpec.configuration.wait_delay
           retry
         end
-      rescue Timeout::Error
-        raise failure || TimeoutError
       end
     end
 

diff --git a/spec/wait_for_spec.rb b/spec/wait_for_spec.rb
@@ -5,7 +5,7 @@
 
   before do
     Thread.new do
-      2.times do
+      11.times do
         sleep 1
         progress << "."
       end
@@ -33,48 +33,40 @@
 
     it "fails if the matcher never passes" do
       expect {
-        wait_for { progress }.to eq("...")
+        wait_for { progress }.to eq("." * 12)
       }.to raise_error(RSpec::Expectations::ExpectationNotMetError)
     end
 
-    it "times out if the block never finishes" do
+    it "passes even if call time exceeds the timeout" do
       expect {
         wait_for {
-          sleep 11
+          sleep 12
           progress
-        }.to eq("..")
-      }.to raise_error(RSpec::Wait::TimeoutError)
+        }.to eq("." * 11)
+      }.not_to raise_error
     end
 
     it "respects a timeout specified in configuration" do
       original_timeout = RSpec.configuration.wait_timeout
       RSpec.configuration.wait_timeout = 3
 
-      begin
-        expect {
-          wait_for {
-            sleep 4
-            progress
-          }.to eq("..")
-        }.to raise_error(RSpec::Wait::TimeoutError)
-      ensure
-        RSpec.configuration.wait_timeout = original_timeout
-      end
+      expect {
+        wait_for { progress }.to eq("." * 4)
+      }.to raise_error(RSpec::Expectations::ExpectationNotMetError)
+    ensure
+      RSpec.configuration.wait_timeout = original_timeout
     end
 
-    it "respects a timeout specified in options", wait: { timeout: 3 } do
+    it "respects a timeout specified in example metadata", wait: { timeout: 3 } do
       expect {
-        wait_for {
-          sleep 4
-          progress
-        }.to eq("..")
-      }.to raise_error(RSpec::Wait::TimeoutError)
+        wait_for { progress }.to eq("." * 4)
+      }.to raise_error(RSpec::Expectations::ExpectationNotMetError)
     end
 
     it "raises an error occuring in the block" do
       expect {
-        wait_for { raise RuntimeError }.to eq("..")
-      }.to raise_error(RuntimeError)
+        wait_for { raise StandardError, "boom" }.to eq("..")
+      }.to raise_error(StandardError, "boom")
     end
 
     it "prevents operator matchers" do
@@ -115,56 +107,36 @@
       }.to raise_error(RSpec::Expectations::ExpectationNotMetError)
     end
 
-    it "times out if the block never finishes" do
+    it "passes even if call time exceeds the timeout" do
       expect {
         wait_for {
-          sleep 11
+          sleep 12
           progress
         }.not_to eq("..")
-      }.to raise_error(RSpec::Wait::TimeoutError)
+      }.not_to raise_error
     end
 
     it "respects a timeout specified in configuration" do
       original_timeout = RSpec.configuration.wait_timeout
       RSpec.configuration.wait_timeout = 3
 
-      begin
-        expect {
-          wait_for {
-            sleep 4
-            progress
-          }.not_to eq("..")
-        }.to raise_error(RSpec::Wait::TimeoutError)
-      ensure
-        RSpec.configuration.wait_timeout = original_timeout
-      end
-    end
-
-    it "respects a timeout specified in options", wait: { timeout: 3 } do
-      expect {
-        wait_for {
-          sleep 4
-          progress
-        }.not_to eq("..")
-      }.to raise_error(RSpec::Wait::TimeoutError)
-    end
-
-    it "raises an error occuring in the block" do
       expect {
-        wait_for { raise RuntimeError }.not_to eq("")
-      }.to raise_error(RuntimeError)
+        wait_for { progress }.not_to match(/\A\.{0,3}\z/)
+      }.to raise_error(RSpec::Expectations::ExpectationNotMetError)
+    ensure
+      RSpec.configuration.wait_timeout = original_timeout
     end
 
-    it "respects the to_not alias when expectation is met" do
+    it "respects a timeout specified in example metadata", wait: { timeout: 3 } do
       expect {
-        wait_for { true }.to_not be(false) # rubocop:disable RSpec/NotToNot
-      }.not_to raise_error
+        wait_for { progress }.not_to match(/\A\.{0,3}\z/)
+      }.to raise_error(RSpec::Expectations::ExpectationNotMetError)
     end
 
-    it "respects the to_not alias when expectation is not met" do
+    it "raises an error occuring in the block" do
       expect {
-        wait_for { true }.to_not be(true) # rubocop:disable RSpec/NotToNot
-      }.to raise_error(RSpec::Expectations::ExpectationNotMetError)
+        wait_for { raise StandardError, "boom" }.not_to eq("")
+      }.to raise_error(StandardError, "boom")
     end
 
     it "prevents operator matchers" do
@@ -178,5 +150,17 @@
         wait_for(progress).not_to eq("..")
       }.to raise_error(ArgumentError, /block/)
     end
+
+    it "respects the to_not alias when expectation is met" do
+      expect {
+        wait_for { true }.to_not eq(false) # rubocop:disable RSpec/NotToNot
+      }.not_to raise_error
+    end
+
+    it "respects the to_not alias when expectation is not met" do
+      expect {
+        wait_for { true }.to_not eq(true) # rubocop:disable RSpec/NotToNot
+      }.to raise_error(RSpec::Expectations::ExpectationNotMetError)
+    end
   end
 end