Repository navigation
[Fix] Prevent NPE and scan starvation in expired token cleanup when the flow rule has been removed - #3664
Open
konlue wants to merge 1 commit into
Open
[Fix] Prevent NPE and scan starvation in expired token cleanup when the flow rule has been removed#3664konlue wants to merge 1 commit into
konlue wants to merge 1 commit into
Conversation
…e flow rule is removed - RegularExpireStrategy#clearToken: null-check the rule looked up by flowId and recycle the token directly when the rule has been removed by a hot update, so the scan keeps going instead of aborting on the same node every round (which starves all following expired tokens and leaks the concurrency counters) - ConcurrentClusterFlowChecker#releaseConcurrentToken: guard against the concurrency counter already being removed by a concurrent rule refresh
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe what this PR does / why we need it
RegularExpireStrategy#clearTokencleans up expired tokens, but it dereferences the rule lookup without a null check:After a rule hot update removes a rule (
ClusterFlowRuleManager#clearAndResetRulesForremoves both the rule and its concurrency counter), staleTokenCacheNodes of that flowId can still remain in the token cache — tokens outlive single requests, and clients release them later. The line above then throws:The impact is worse than the NPE itself. The cleanup loop scans from the head of the key list every round, the exception is swallowed by
ClearExpiredTokenTask#run's catch block, and the offending element is never removed (onlyremoveTokenremoves it, which is never reached). So every subsequent round aborts on the same node:This PR adds the missing guards so the cleanup task keeps working across rule deletions.
Does this pull request fix one issue?
Fixes #3663
Describe how you did it
RegularExpireStrategy#clearToken: take the rule into a local variable; whengetFlowRuleByIdreturns null, the token can never be released by its client anymore (the release path needs the rule), so recycle it directly viaremoveToken(key, node)andcontinue— the scan keeps going and the concurrency counter is released through the usual path.ConcurrentClusterFlowChecker#releaseConcurrentToken: guard the same-origin race — inside the rule-refresh window the concurrency counter can already have been removed byclearAndResetRulesFor, soCurrentConcurrencyManager.get(flowId)may return null; in that case log and returnRELEASE_OKinstead of NPE-ing atnowCalls.getAndAdd(...).Both guards follow the conventions of the sibling code paths, which already null-check the same sources (
removeTokenchecksCurrentConcurrencyManager.get(...);releaseConcurrentTokenchecksgetFlowRuleById). This PR only fills in the two missing spots.Describe how to verify it
Added
RegularExpireStrategyNpeTest(JUnit 4, plain in-memory, no real cluster server needed), 3 cases:clearTokenShouldNotThrowWhenRuleHasBeenDeleted: the rule has been removed by a hot update → cleanup must not throw and the token must be recycled. Before this fix it fails with the NPE shown above.clearTokenShouldContinueScanningBehindNodeWithAbsentRule: an expired token located behind an absent-rule node must also be recycled, and its concurrency counter must be released (5 → 4 with acquireCount=1). Before this fix the scan aborts on the absent-rule node and this token starves forever.releaseConcurrentTokenShouldNotThrowWhenCounterHasBeenRemoved: the counter has already been removed by a concurrent rule refresh → the release path must not throw and must returnRELEASE_OK.Run
mvn test -DskipTests=falseonsentinel-cluster-server-default: 3/3 green with this fix (red without it), and the full module suite passes with no regression.Special notes for reviews
releaseConcurrentTokenreturnsRELEASE_OK; a dedicated status would extend the publicTokenResultStatuscontract, so I kept the smaller change.