beatlabs/patron

Retry Pattern - Unexpected Behavior

pkritiotis opened this issue · 1 comments

pkritiotis commented

Is your feature request related to a problem? Please describe

The Retry utility pattern implemented here has unexpected behavior.

  1. There is one bug in an edge-case scenario: When the retrials is set to 0, there is a bug that causes the code to enter an infinite loop: https://github.com/beatlabs/patron/blob/master/reliability/retry/retry.go#L37-L40
  2. The term retrials in combination with the constructor accepting 0 as valid input is confusing.
    • We accept 0 as a valid number of retrials, and the algorithm implemented always triggers at least one execution
    • Assuming this is expected, it means that retrials represents the extra attempts only, not the total attempts
    • This means that if we passed retrials=1 I would expect the utility to
      1. Run the first time
      2. If it fails, retry.
      3. If it fails again return an error
    • Currently the following happens:
      1. Runs the first time
      2. If it failed, return an error

Is this a bug? Please provide steps to reproduce, a failing test etc.

Yes, it's a bug. Tests to reproduce:

For Bug (1) the following tests fail in the current implementation:

 		"error after a failure with zero retries": {. // this executes the action twice
			retries:            0,
			action:             mockAction{errors: 1},
			expectedExecutions: 1,
			expectErr:          true,
		},
		"error after zero retries should execute only once": { //this executes the action 10 times - when the action is successful
			retries:            0,
			action:             mockAction{errors: 9},
			expectedExecutions: 1,
		},

For the behavior explained in point (2):

  • Either we don't need to accept 0 for retrials or the following test case is wrong 
    •      //Current
	  "error": {
		  retries:            3,
		  action:             mockAction{errors: 3},
		  expectedExecutions: 3,
		  expectErr:          true,
	  },

	//Expected
	  "error": {
		  retries:            3,
		  action:             mockAction{errors: 4},
		  expectedExecutions: 4,
		  expectErr:          true,
	  },

Describe the solution

I think from a readability perspective, it is confusing to think of retrials separately from the first attempt. My suggestions:

  1. The term attempts might be more clear for the users of this utility. attempts represents the total number of attempted executions, including the first one
  2. The constructor will accept only attempts > 1
    • For 1 attempt, the retry pattern does not provide any value

I have made the above changes here and I can proceed with a PR if this is something we agree on

Note: Considering that this implementation has a buggy behavior, I don't know whether we can consider this as a breaking change

mantzas commented

Very nice catch!!! Let's fix this with a PR.

  • 👍1