ExponentialBackOffPolicy and BackOffContext [BATCH-1795]

[spring-projects-issues]

Kevin Chan opened BATCH-1795 and commented

Hi,

I configure a ExponentialBackOffPolicy like this:

<bean id="exponentialBackOffPolicy" class="org.springframework.batch.retry.backoff.ExponentialBackOffPolicy"
    p:initialInterval="500" p:maxInterval="30000" p:multiplier="2" />

and use it for DeadLockLoserDataAccessException. Then I intentionally throw a DeadLockLoserDataAccessException in the main part of my item processor code and observe the behaviour. However, the retry never backoffs exponentially. I trace through the RetryTemplate class (in particulary lines 197 - 256 in 2.1.7 release), and the backOffPolicy and its context seem stateless in runtime:

    protected <T> T doExecute(RetryCallback<T> retryCallback, RecoveryCallback<T> recoveryCallback, RetryState state)
            throws Exception, ExhaustedRetryException {

        RetryPolicy retryPolicy = this.retryPolicy;
        BackOffPolicy backOffPolicy = this.backOffPolicy;

...
        // Start the backoff context...
        BackOffContext backOffContext = backOffPolicy.start(context);
...
        backOffPolicy.backOff(backOffContext);
...

That is, the backOffPolicy is never updated and the backOffContext is never saved after the call to backoff, hence the interval stays the same value.

I submitted the same issue to the Spring forum and StackOverflow and no answer, hence created this issue.

Thanks, Kev

---

Affects: 2.1.7

Reference URL: http://forum.springsource.org/showthread.php?114517-ExponentialBackOffPolicy-and-BackOffContext&p=379544#post379544

Issue Links:

• AMQP-226 Retry Exponential BackOff Policy Doesn't Back Off

Referenced from: commits https://github.com/spring-projects/spring-batch/commit/b850021187ff7503ed02ef715949c2be26ae920e

Backported to: 2.2.0 - Sprint 9

[spring-projects-issues]

Dave Syer commented

Wow, that's an old, old bug. I'm amazed we never spotted that before, so thanks for the input.

The problem is that the RetryCache only stores the RetryContext, which does not contain the BackoffContext. I think it might require some hackery to fix it without changing the RetryContext interface, so it's going to be tricky. Suggestions warmly accepted. Maybe a RetryContext wrapper that includes the original context and the backoff context would work (so changes only internal to RetryTemplate)? If you want to contribute a pull request, please feel free. You could send it to http://github.com/spring-retry and we could backport to Spring Batch if you can find a non breaking change.

[spring-projects-issues]

Kevin Chan commented

Hi Dave, I added a backOffContext member in the RetryContextSupport, and changed the ExponentialBackOffPolicy start() method to lookup from/set the backOffContext into the RetryContextSupport.

I made the changes to the spring-batch-infrastructure, built and tested it in my project. It works. Before I port the same changes to spring-retry, can you please do a code review?

The diff is below.

Thanks, Kev.

In ExponentialBackOffPolicy.java

diff --git a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/backoff/ExponentialBackOffPolicy.java b/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/backoff/ExponentialBackOffPolicy.java
index 594d2da..ae9c7a8 100644
--- a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/backoff/ExponentialBackOffPolicy.java
+++ b/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/backoff/ExponentialBackOffPolicy.java
@@ -17,6 +17,7 @@
 package org.springframework.batch.retry.backoff;

 import org.springframework.batch.retry.RetryContext;
+import org.springframework.batch.retry.context.RetryContextSupport;
 import org.springframework.util.ClassUtils;

 /**
@@ -140,8 +141,31 @@ public class ExponentialBackOffPolicy implements BackOffPolicy {
     * Returns a new instance of {@link BackOffContext} configured with the
     * 'expSeed' and 'increment' values.
     */
-   public BackOffContext start(RetryContext context) {
-       return new ExponentialBackOffContext(this.initialInterval, this.multiplier, this.maxInterval);
+   public BackOffContext start(RetryContext context) 
+   {
+        BackOffContext backOffContext;
+
+        /* If the RetryContext is RetryContextSupport, then get the BackOffContext
+         * from there, or 'persist' a new BackOffContext to it if none is available. */
+        if (RetryContextSupport.class.isAssignableFrom(context.getClass())) 
+        {
+            RetryContextSupport backOffContextAware = (RetryContextSupport) context;
+            backOffContext = backOffContextAware.getBackOffContext();
+            
+            if (backOffContext == null) 
+            {
+                backOffContext = new ExponentialBackOffContext(this.initialInterval, this.multiplier, this.maxInterval);
+                backOffContextAware.setBackOffContext(backOffContext);
+            }
+        }
+        /* Not a RetryContextSupport, create a new BackOffContext. Note, that 
+         * means the context is never saved and all the retries will happen
+         * in constant (initial) interval, which makes this exponential meaningless. */
+        else 
+        {
+            backOffContext = new ExponentialBackOffContext(this.initialInterval, this.multiplier, this.maxInterval);
+        }
+        return backOffContext;
    }

In RetryContextSupport.java

diff --git a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/context/RetryContextSupport.java b/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/context/RetryContextSupport.java
index 321c67e..bffe210 100644
--- a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/context/RetryContextSupport.java
+++ b/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/context/RetryContextSupport.java
@@ -18,8 +18,10 @@ package org.springframework.batch.retry.context;

 import org.springframework.batch.retry.RetryContext;
 import org.springframework.batch.retry.RetryPolicy;
+import org.springframework.batch.retry.backoff.BackOffContext;
 import org.springframework.core.AttributeAccessorSupport;

+
 public class RetryContextSupport extends AttributeAccessorSupport implements RetryContext {

    private boolean terminate = false;
@@ -30,6 +32,8 @@ public class RetryContextSupport extends AttributeAccessorSupport implements RetryContext

    private RetryContext parent;

+    private BackOffContext backOffContext;
+
    public RetryContextSupport(RetryContext parent) {
        super();
        this.parent = parent;
@@ -55,7 +59,17 @@ public class RetryContextSupport extends AttributeAccessorSupport implements Ret
        return lastException;
    }

-   /**
+    public BackOffContext getBackOffContext()
+    {
+        return backOffContext;
+    }
+
+    public void setBackOffContext(BackOffContext backOffContext)
+    {
+        this.backOffContext = backOffContext;
+    }
+
+    /**

[spring-projects-issues]

Dave Syer commented

I guess that works. We need a test case, and the comment about meaninglessness is not strictly true (it all works fine for stateless retry). Can you send a pull request, please?

[spring-projects-issues]

Dave Syer commented

This is fixed in spring-retry, so in 2.2 Batch will pick up that change.

[spring-projects-issues]

Michael Minella commented

Since this is really a spring retry issue, resolving per Dave's comment.