Skip to content

Fix FlowRuleComparator violating the Comparator contract #3629

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

Open
vasiliy-mikhailov wants to merge 1 commit into
base: 1.8
Choose a base branch
from
+46 −2
Open

Fix FlowRuleComparator violating the Comparator contract #3629

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions ...inel-core/src/main/java/com/alibaba/csp/sentinel/slots/block/flow/FlowRuleComparator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,15 @@ public int compare(FlowRule o1, FlowRule o2) {
return -1;
}

if (o1.getLimitApp() == null) {
if (o1.getLimitApp() == null && o2.getLimitApp() == null) {
return 0;
}
if (o1.getLimitApp() == null) {
return 1;
}
if (o2.getLimitApp() == null) {
return -1;
}

if (o1.getLimitApp().equals(o2.getLimitApp())) {
return 0;
Expand All @@ -50,7 +56,7 @@ public int compare(FlowRule o1, FlowRule o2) {
} else if (RuleConstant.LIMIT_APP_DEFAULT.equals(o2.getLimitApp())) {
return -1;
} else {
return 0;
return o1.getLimitApp().compareTo(o2.getLimitApp());

@oss-sentinel-ai oss-sentinel-ai Sep 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified the surrounding semantics: FlowRuleUtil.buildFlowRuleMap sorts per-resource rule lists with this comparator. Origin-specific rules still sort before LIMIT_APP_DEFAULT and cluster-mode rules still sort last, so rule matching order is preserved; the new lexicographic tie-break between two different non-default limitApp values only makes the ordering deterministic. Combined with FlowRuleUtil normalizing blank limitApp to default before sorting, this safely fixes the Comparator contract violations (antisymmetry) that could trigger IllegalArgumentException from TimSort on rule updates.

}
}

Expand Down
38 changes: 38 additions & 0 deletions ...-core/src/test/java/com/alibaba/csp/sentinel/slots/block/flow/FlowRuleComparatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,4 +42,42 @@ private void assertOrderEqual(int size, List<FlowRule> expected, List<FlowRule>
assertEquals(expected.get(i), actual.get(i));
}
}

@Test
public void testNullVsNonNullLimitApp() {
FlowRuleComparator comparator = new FlowRuleComparator();

// o1 has null limitApp, o2 has non-null limitApp
FlowRule ruleNull = new FlowRule("abc");
ruleNull.setLimitApp(null);
FlowRule ruleNonNull = new FlowRule("abc");
ruleNonNull.setLimitApp("originA");

// Comparator should NOT return 0 when one is null and the other is not
int result = comparator.compare(ruleNull, ruleNonNull);
assertNotEquals("null vs non-null should not be equal", 0, result);

// Symmetry: compare(o2, o1) should be -compare(o1, o2)
int reverseResult = comparator.compare(ruleNonNull, ruleNull);
assertEquals("comparator must be antisymmetric", -result, reverseResult);
}

@Test
public void testDifferentNonDefaultLimitApps() {
FlowRuleComparator comparator = new FlowRuleComparator();

// Two rules with different non-DEFAULT limitApp values
FlowRule ruleA = new FlowRule("abc");
ruleA.setLimitApp("originA");
FlowRule ruleB = new FlowRule("abc");
ruleB.setLimitApp("originB");

// Different apps should NOT compare as equal (0)
int result = comparator.compare(ruleA, ruleB);
assertNotEquals("different non-default apps should not be equal", 0, result);

// Symmetry check
int reverseResult = comparator.compare(ruleB, ruleA);
assertEquals("comparator must be antisymmetric", -result, reverseResult);
}
}