Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Add `total_delay` to handler arguments #1

Merged
merged 2 commits into from

2 participants

@mmazour

Pass the total time waited to the handler. This is to facilitate, for example,
bumping a stat or emitting a warning when delays exceed a threshold.

Includes test, and README change.

Michael Mazour Add `total_delay` to handler arguments
Pass the total time waited to the handler.  This is to facilitate, for example,
bumping a stat or emitting a warning when delays exceed a threshold.
ea1a653
@cespare
Collaborator

You may also find Minitest's assert_in_delta useful here.

@cespare
Collaborator

Minor style quibble: can you chop off long lines to be at most 110 characters? (This one and 85).

@cespare
Collaborator

It's a pretty minor thing, but before this change the tests ran in about 0.001s for me, and now they take about 0.6s. Can you write the total_delay test in such a way that it doesn't contribute too much to the running time?

@cespare
Collaborator

In addition to updating the readme, can you please update the yardoc method comment where with_retries is declared? Thanks.

@cespare
Collaborator

Hi @mmazour, thanks for this change. See the comments I've made -- otherwise, it looks good to me. I'll merge and push a new gem version if you can address the comments.

Michael Mazour Further tweaks to `total_delay` handler arg
- Improve method comment
- Tidy up test
 - Speed up test
5e700e0
@mmazour

Thanks very much, @cespare. I've now added a commit with the changes you suggested. (I also wrapped a line in one of your tests to make it visually consistent with the line of mine that had to be wrapped; hope that's OK.)

Tests now run in ~ 0.11s - 0.14s for me; is that quick enough? I can dial the timings down tighter if you like, but I didn't want to push too hard and wind up with false failures on (for example) a slightly hiccup-prone older laptop.

@cespare cespare merged commit 5e700e0 into from
@cespare
Collaborator

Hi @mmazour,

I merged the change. I went ahead and made the test completely timing-independent in d865b74; let me know what you think. I've also pushed 0.0.4 to Rubygems.

@mmazour
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 11, 2013
  1. Add `total_delay` to handler arguments

    Michael Mazour authored
    Pass the total time waited to the handler.  This is to facilitate, for example,
    bumping a stat or emitting a warning when delays exceed a threshold.
Commits on Mar 12, 2013
  1. Further tweaks to `total_delay` handler arg

    Michael Mazour authored
    - Improve method comment
    - Tidy up test
     - Speed up test
This page is out of date. Refresh to see the latest.
Showing with 31 additions and 13 deletions.
  1. +10 −9 README.md
  2. +4 −2 lib/retries.rb
  3. +17 −2 test/retries_test.rb
View
19 README.md
@@ -48,12 +48,13 @@ end
### Handlers
`with_retries` allows you to pass a custom handler that will be called each time before the block is retried.
-The handler will be called with two arguments: `exception` (the rescued exception) and `attempt_number` (the
-number of attempts that have been made thus far).
+The handler will be called with three arguments: `exception` (the rescued exception), `attempt_number` (the
+number of attempts that have been made thus far), and `total_delay` (the number of seconds since the start
+of the time the block was first attempted, including all retries).
``` ruby
-handler = Proc.new do |exception, attempt_number|
- puts "Handler saw a #{exception.class}; retry attempt #{attempt_number}"
+handler = Proc.new do |exception, attempt_number, total_delay|
+ puts "Handler saw a #{exception.class}; retry attempt #{attempt_number}; #{total_delay} seconds have passed."
end
with_retries(:max_tries => 5, :handler => handler, :rescue => [RuntimeError, ZeroDivisionError]) do |attempt|
(1 / 0) if attempt == 3
@@ -61,13 +62,13 @@ with_retries(:max_tries => 5, :handler => handler, :rescue => [RuntimeError, Zer
end
```
-This will print:
+This will print something like:
```
-Handler saw a RuntimeError; retry attempt 1
-Handler saw a RuntimeError; retry attempt 2
-Handler saw a ZeroDivisionError; retry attempt 3
-Handler saw a RuntimeError; retry attempt 4
+Handler saw a RuntimeError; retry attempt 1; 2.9e-05 seconds have passed.
+Handler saw a RuntimeError; retry attempt 2; 0.501176 seconds have passed.
+Handler saw a ZeroDivisionError; retry attempt 3; 1.129921 seconds have passed.
+Handler saw a RuntimeError; retry attempt 4; 1.886828 seconds have passed.
```
### Delay parameters
View
6 lib/retries.rb
@@ -18,7 +18,8 @@ module Kernel
# @option options [Float] :base_sleep_seconds (0.5) The starting delay between retries.
# @option options [Float] :max_sleep_seconds (1.0) The maximum to which to expand the delay between retries.
# @option options [Proc] :handler (nil) If not `nil`, a `Proc` that will be called for each retry. It will be
- # passed two arguments, `exception` (the rescued exception) and `attempt_number`.
+ # passed three arguments, `exception` (the rescued exception), `attempt_number`,
+ # and `total_delay` (seconds since start of first attempt).
# @option options [Exception, <Exception>] :rescue (StandardError) This may be a specific exception class to
# rescue or an array of classes.
# @yield [attempt_number] The (required) block to be executed, which is passed the attempt number as a
@@ -40,12 +41,13 @@ def with_retries(options = {}, &block)
# Let's do this thing
attempts = 0
+ start_time = Time.now
begin
attempts += 1
return block.call(attempts)
rescue *exception_types_to_rescue => exception
raise exception if attempts >= max_tries
- handler.call(exception, attempts) if handler
+ handler.call(exception, attempts, Time.now - start_time) if handler
# Don't sleep at all if sleeping is disabled (used in testing).
if Retries.sleep_enabled
# The sleep time is an exponentially-increasing function of base_sleep_seconds. But, it never exceeds
View
19 test/retries_test.rb
@@ -66,8 +66,8 @@ class RetriesTest < Scope::TestCase
assert_equal exception_handler_run_times, attempt_number
assert exception.is_a?(CustomErrorA)
end
- with_retries(:max_tries => 4, :base_sleep_seconds => 0, :max_sleep_seconds => 0, :handler => handler,
- :rescue => CustomErrorA) do
+ with_retries(:max_tries => 4, :base_sleep_seconds => 0, :max_sleep_seconds => 0,
+ :handler => handler, :rescue => CustomErrorA) do
tries += 1
raise CustomErrorA.new if tries < 4
end
@@ -75,6 +75,21 @@ class RetriesTest < Scope::TestCase
assert_equal 3, exception_handler_run_times
end
+ should "pass total elapsed time to :handler upon each handled exception" do
+ exception_handler_run_times = 0
+ tries = 0
+ start_time = Time.now
+ handler = Proc.new do |exception, attempt_number, total_delay|
+ # Check that the handler is passed the proper total delay time
+ assert_in_delta(total_delay, Time.now - start_time, 0.01)
+ end
+ with_retries(:max_tries => 3, :base_sleep_seconds => 0.05, :max_sleep_seconds => 0.25,
+ :handler => handler, :rescue => CustomErrorA) do
+ tries += 1
+ raise CustomErrorA.new if tries < 3
+ end
+ end
+
should "not sleep if Retries.sleep_enabled is false" do
Retries.sleep_enabled = false
assert_raises(RuntimeError) do # If we get a Timeout::Error, this won't pass.
Something went wrong with that request. Please try again.