Repository navigation
fix: preserve shared parameter thread counters when removing a rule - #3657
hutiefang76 wants to merge 1 commit into
Conversation
konlue
left a comment
There was a problem hiding this comment.
Reviewed the full diff and verified locally in a worktree on 1.8 (a3f40ba): with the patch, the parameter-flow module and its reactor dependencies pass 268 tests; reverting only ParameterMetric.java (and ParamFlowRuleUtil.java for the sibling PR) turns all four new tests red with exactly the assertions they claim to pin (expected same:<...> was not:<null> for the shared counter, expected:<2> but was:<0> through the real loadRules reload path).
What I think is done well:
- The self-exclusion ordering is correct: the rule's own time/token counters are removed before
isParamIdxInUsescans for other users of the param index, so a cleared rule can never keep its own counter alive. - The tests cover three layers (unit keep, unit remove-when-unique, manager reload integration), and the reload test exercises the real
ParamFlowRuleManager#loadRulespath rather than callingclearForRuledirectly. threadCountMapis indeed shared per param index across rules of the same resource, so the issue's premise holds.
One boundary worth discussing, none of it blocking:
The in-use check only sees initialized rules. ruleTimeCounters/ruleTokenCounter are populated by initialize(), which runs lazily on the first checked request. Two consequences I traced:
- A loaded-but-never-hit sibling rule sharing the param index won't keep the counter alive; when its first request initializes it, a fresh empty
CacheMapis created, and in-flight counts from the removed rule's era are gone (subsequent decrements can transiently drive a fresh counter negative via theputIfAbsent/decrementAndGetdance before it self-heals — i.e. the surviving rule under-counts for a while). ParamFlowRule#equalsis full value semantics (count included), and the reload path derives removed rules viaoldRuleList.removeAll(newRuleList). So a rule whosecountwas merely updated counts as "removed + re-added"; if it shares a param index with a genuinely removed sibling that is processed after it in the cleanup loop, the shared counter is dropped even though a rule for that resource + index logically survives.
Neither is a regression against today's unconditional removal, and consulting the manager's loaded rules from ParameterMetric would break the layering — so at minimum I'd suggest a short comment on isParamIdxInUse documenting the "initialized rules only" boundary. If you want to close the reload-path gap, checking the new rule map (resource + paramIdx) before dropping the counter would be the natural place, since the manager already has it in scope.
Happy to help with tests for either edge if you want to pursue it.
Describe what this PR does / why we need it
ParameterMetric.clearForRule unconditionally removes the threadCountMap entry for a parameter index. Two initialized rules can share that index, so deleting one rule also discards the surviving rule's in-flight thread counts.
Does this pull request fix one issue?
Fixes #3655.
Describe how you did it
Remove the deleted rule caches first, then discard the shared index counter only when no initialized rule still uses that index. Add direct lifecycle and manager reload regressions, including unique-index and last-rule cleanup.
Describe how to verify it
JDK 17: the parameter-flow module and its reactor dependencies passed 266 tests on this PR branch. The old shared-counter implementation fails the direct and real-manager reload assertions. Java 8 execution was attempted but blocked by the existing compiler/toolchain bytecode mismatch; no Java 8 pass is claimed.
mvn -q -pl sentinel-extension/sentinel-parameter-flow-control -am testSpecial notes for reviews
AI assistance: implementation and initial review used DeepSeek; Codex coordinated final review and verification.