Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Backoff annotation 'delayExpression' attribute is not always applied #332

Closed
pstelzer1 opened this issue Feb 10, 2023 · 2 comments
Closed

Comments

@pstelzer1
Copy link

An annotation like this

   @Retryable(maxAttemptsExpression = "${application.config.maxRetryAttempts}",
      backoff = @Backoff(delayExpression = "${application.config.retryDelayTimeMs}"),
      retryFor = {MyRetryableException.class})
   public void myMethod(){
   ...
   }

does not result in a backoff delay pulled from the delayExpression as intended, but rather uses the default delay of 1000ms.

Debugging through the code, BackoffPolicyBuilder.build() correctly receives the delaySupplier with the value pulled from ${application.config.retryDelayTimeMs}, but only applies this field if a multiplier is present, or if a maxDelay is specified. If neither of these are true, then a FixedBackoffPolicy is instantiated, populated with the default delay of 1000ms and a sleeper, and returned.

FixedBackoffPolicy can support a backoff period supplier, but none is set in the BackoffPolicyBuilder.build() method.

This can be worked around by adding a multiplier of 1, or by adding a maxDelay to the backoff annotation, thus directing the BackoffPolicyBuilder to the sections of the build() method that take into account the delay expression. However, seems like a FixedBackoffPolicy can be generated that uses the specified delay expression, without having to set these other annotation attributes.

@artembilan
Copy link
Member

Confirmed. This is a bug in the BackOffPolicyBuilder:

		FixedBackOffPolicy policy = new FixedBackOffPolicy();
		if (this.delay != null) {
			policy.setBackOffPeriod(this.delay);
		}
		if (this.sleeper != null) {
			policy.setSleeper(this.sleeper);
		}

As you see we don't take into account a this.delaySupplier as we do for other policies above.

Feel free to provide a contribution.

Thank you!

@pstelzer1
Copy link
Author

This turns out to not be a bug.

Expressions are parsed correctly prior to invoking the BackoffPolicyBuilder, specifically in AnnotationAwareRetryOperationsInterceptor.getBackoffPolicy() line 408-414 and 418-424, but for spel expressions wrapped in #{...}, not for property expressions ${...}. (See the isTemplate() method.) If the property expression is wrapped in #{...}, then the expression is parsed correctly. Here is an example.

   @Retryable(maxAttemptsExpression = "#{${application.config.maxRetryAttempts}}",
      backoff = @Backoff(delayExpression = "#{${application.config.retryDelayTimeMs}}"),
      retryFor = {MyRetryableException.class})
   public void myMethod(){
   ...
   }

No fixes needed - the code works correctly once I set up the retry and backup expressions correctly.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants