Skip to content

fix: preserve shared parameter thread counters when removing a rule - #3657

Open
hutiefang76 wants to merge 1 commit into
alibaba:1.8from
hutiefang76:frank/sentinel-nextten-20261007-01a11186
Open

hutiefang76 wants to merge 1 commit into
alibaba:1.8from
hutiefang76:frank/sentinel-nextten-20261007-01a11186

Conversation

@hutiefang76

@hutiefang76 hutiefang76 commented Oct 7, 2026 •

Copy link
Copy Markdown

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 test

Special notes for reviews

AI assistance: implementation and initial review used DeepSeek; Codex coordinated final review and verification.

@konlue konlue left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 isParamIdxInUse scans 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#loadRules path rather than calling clearForRule directly.
  • threadCountMap is 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:

  1. 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 CacheMap is created, and in-flight counts from the removed rule's era are gone (subsequent decrements can transiently drive a fresh counter negative via the putIfAbsent/decrementAndGet dance before it self-heals — i.e. the surviving rule under-counts for a while).
  2. ParamFlowRule#equals is full value semantics (count included), and the reload path derives removed rules via oldRuleList.removeAll(newRuleList). So a rule whose count was 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.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Removing one parameter rule clears thread counts still used by another rule

2 participants