Skip to content

Issue #2295: Ensure ignoreExceptions take precedence over recordExceptions #2304

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 23, 2025
Merged

Issue #2295: Ensure ignoreExceptions take precedence over recordExceptions #2304

merged 1 commit into from
May 23, 2025

Conversation

kssumin
Copy link
Contributor

@kssumin kssumin commented Apr 6, 2025 •
edited
Loading

Problem

In the current behavior of CircuitBreakerConfig, exception handling may not fully align with user expectations when both recordExceptions and ignoreExceptions are configured.

For example, if a subclass is listed in ignoreExceptions, but its superclass is also included in recordExceptions, the subclass may still be recorded.
This can make it difficult to selectively ignore specific exceptions while keeping track of their more general types.

This can lead to behavior that doesn’t match user expectations — for example, when users want to ignore specific exceptions while still tracking more general ones.

Expected Behavior

Exceptions listed in ignoreExceptions should never be recorded, even if they are subclasses of exceptions defined in recordExceptions.

Changes Introduced

Updated the CircuitBreakerConfig.Builder.build() method to ensure that the recordExceptionPredicate always checks ignoreExceptionPredicate first.

The final predicate now ensures that if an exception matches the ignore list, it will be excluded from recording regardless of any match in the record list.

Predicate<Throwable> baseRecordExceptionPredicate = createRecordExceptionPredicate();
config.recordExceptionPredicate = throwable ->
    !config.ignoreExceptionPredicate.test(throwable) && baseRecordExceptionPredicate.test(throwable);

Testing

Added unit tests to verify correct handling of exception hierarchy.

Confirmed that specific exceptions in ignoreExceptions are not recorded even when their superclass is in recordExceptions.

Summary

This change ensures consistent and predictable behavior when both recordExceptions and ignoreExceptions are configured. It guarantees that ignored exceptions always take precedence, providing better control over which failures are recorded.

Related Issue

Fixes #2295

@RobWin When you have a moment, I’d love your feedback on this fix.
It addresses how ignoreExceptions and recordExceptions interact and ensures the intended priority behavior. Thanks in advance!

@dosubot dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Apr 6, 2025
@kssumin kssumin changed the title fix: Ensure ignoreExceptions take precedence over recordExceptions Issue #2295: Ensure ignoreExceptions take precedence over recordExceptions Apr 6, 2025
@dosubot dosubot bot added the bug label Apr 6, 2025
@kssumin kssumin marked this pull request as draft April 6, 2025 09:34
@kssumin kssumin marked this pull request as ready for review April 6, 2025 09:34
@dosubot dosubot bot added enhancement java Pull requests that update Java code labels Apr 6, 2025
@kssumin kssumin marked this pull request as draft April 17, 2025 03:58
@kssumin kssumin marked this pull request as ready for review April 20, 2025 10:41
@RobWin
Copy link
Member

RobWin commented May 7, 2025

Thank you.
From what you wrote it total makes sense.
I just wonder how we can stay backwards compatible, otherwise this change might effect the configuration of current users or?

@kssumin
Copy link
Contributor Author

kssumin commented May 18, 2025

@RobWin
Thank you for your review and feedback!

You raise a valid concern about backward compatibility. This change fixes a logical inconsistency where the ignoreExceptions configuration wasn't properly taking precedence over recordExceptions.

As for backward compatibility impact:

  1. This change only affects cases where both recordExceptions and ignoreExceptions are configured, AND an exception occurs that matches both criteria. In these specific cases, the exception will now be properly ignored rather than recorded.

  2. From a user expectation standpoint, I believe this aligns better with what users would intuitively expect - if they explicitly configure an exception to be ignored, it should override recordExceptions.

For maintaining backward compatibility, we could:

  • Document this clearly in release notes as a bugfix that corrects exception handling priority
  • If you feel it's necessary, we could consider adding a configuration flag (like legacyExceptionHandling) to preserve the old behavior, though I think the new behavior is more correct

I'm open to any other suggestions you might have to address backward compatibility concerns while fixing this logical issue.

@RobWin
Copy link
Member

RobWin commented May 22, 2025 •
edited
Loading

Yes, I think a flag to preserve backwards compability would be helpful, if it is not too much effort.

@kssumin
fix: Ensure ignoreExceptions takes precedence over recordExceptions
21df745 
Signed-off-by: Kim Sumin <ksoomin25@gmail.com>
@kssumin kssumin force-pushed the fix-exception-priority branch from cf50997 to 21df745 Compare May 23, 2025 05:13
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels May 23, 2025
@kssumin
Copy link
Contributor Author

kssumin commented May 23, 2025

Yes, I think a flag to preserve backwards compability would be helpful, if it is not too much effort.

@RobWin Thank you for the feedback! I've implemented the backward compatibility flag as requested.

I added a new field:

private boolean ignoreExceptionsPrecedenceEnabled = false;

With corresponding builder methods:

public Builder ignoreExceptionsPrecedenceEnabled(boolean enable) { ... }
public Builder enableIgnoreExceptionsPrecedence() { ... }

Behavior:

  • false (default): Legacy behavior - recordExceptions takes precedence
  • true: New behavior - ignoreExceptions takes precedence over recordExceptions

Question

How do you feel about the field name ignoreExceptionsPrecedenceEnabled?
(current choice - matches automaticTransitionFromOpenToHalfOpenEnabled)

The current name follows the existing {feature}Enabled pattern in the codebase.
Would you prefer a different name, or does this work well for you?

@RobWin
Copy link
Member

RobWin commented May 23, 2025

Hi, thank you.
I think the name is fine. JavaDoc explains it well.

@RobWin RobWin merged commit 7d6895d into resilience4j:master May 23, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug enhancement java Pull requests that update Java code size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Issue with recordExceptionPredicate and ignoreExceptionPredicate for WebClientResponseException variants in Circuit Breaker configuration
2 participants
@kssumin @RobWin