Repository navigation
Conversation
Aias00
left a comment
There was a problem hiding this comment.
Approved. The fallback path was the real offender and this fixes it properly.
Mono.just(AhoCorasick.of(words)) evaluated AhoCorasick.of eagerly at assembly time, on whatever thread assembled the chain — in onErrorResume that is the request thread, i.e. the Netty event loop. The redis path at least had subscribeOn(boundedElastic), but subscribeOn only moves the subscription upstream, so the map that builds the automaton still ran on the redis driver threads. Adding publishOn(boundedElastic) before the map is exactly the operator that moves the CPU-bound stage downstream, and Mono.fromCallable(...).subscribeOn(boundedElastic) is the correct way to defer a synchronous build.
Both new tests are discriminating rather than decorative, which is what makes this reviewable:
testTheDictionaryIsBuiltOffTheRedisIoThreadsdrives the source on a dedicatedsimulated-redis-ioparallel scheduler and asserts the build thread name does not start with that prefix. WithoutpublishOnthis test fails, because themapwould run on the emitting thread — so it pins the actual defect.testTheRuleWordsAutomatonBuildIsDeferredasserts that assemblingruleWordsAutomaton(...)does not invokeAhoCorasick.ofat all, then verifies the deferred build still works on subscription. That is precisely the eager-evaluation bug.
Three notes, none blocking:
-
The fallback Mono is cold and rebuilds per subscription.
Mono.fromCallablere-executes on every subscribe where the oldMono.justreused one instance. In the current flowdictionary()is assembled per request and subscribed once, so the count is unchanged — and the result is still not cached in the fallback branch (only the redis branch callscachedHandle). If the chain is ever retried or resubscribed, the automaton would be rebuilt each time off-loop. Worth a comment saying that is expected. -
Two scheduler hops.
subscribeOn(boundedElastic)pluspublishOn(boundedElastic)means the subscription and the emission each cross a bounded-elastic queue. Reactive Redis is non-blocking, so subscribing from the caller thread is safe and thesubscribeOnis arguably redundant now thatpublishOnguards the CPU stage. Not a correctness issue — just a hop worth dropping if this path ever shows up in profiles. -
Visibility.
redisAutomatonandruleWordsAutomatonare package-private only so the test can reach them.privateplus@VisibleForTesting, or exercising them throughdictionary()/doExecute, would keep the plugin’s surface tighter. Minor.
CI has not reported on this branch yet — please confirm the workflow passes before merge.
|
Follow-up performance fix for the same method is stacked on this branch and already open as #7850 — its diff currently includes this PR's changes; that portion will drop out automatically once this merges. |
|
@konlue pls fix ci |
|
Heads-up: this PR currently contains no changes relative to The branch is two commits ahead (the fix plus the "Merge master … to fix CI" commit), but the resulting tree is identical to the base, which means the sensitive-word dictionary change already reached Could you confirm that the fix you intended is the one now on |
|
Confirmed — that's correct. The dictionary-threading fix reached |
Fixes #7830.
Summary
SensitiveWordPlugin#dictionaryrelied onsubscribeOn(boundedElastic()), which only moves the subscription point: the redis driver still emits on its own io threads, so the cpu-bound Aho-Corasick construction ran there. TheonErrorResumefallback built the rule-words automaton inline on the error thread as well.Modifications
publishOn(Schedulers.boundedElastic())betweencollectList()and the automaton construction.Mono.fromCallable+subscribeOn, so it never runs on the error-emitting thread.redisAutomaton,ruleWordsAutomaton) to make the threading directly testable.Verifying this change
testTheDictionaryIsBuiltOffTheRedisIoThreads: the redis source is pinned to a namedsimulated-redis-ioscheduler; the construction thread must be a bounded elastic worker. Fails on the current code (build lands on the io thread), passes with the fix.testTheRuleWordsAutomatonBuildIsDeferred: assembling the fallback automaton must not invoke the build (thread-local static interception captures nothing at assembly time); the deferred build still completes on subscription.Red-green: reverting only the fix internals (keeping the extracted seams) fails both tests; restoring them passes. Full
SensitiveWordPluginTest18/18, no regressions.Targeted tests pass:
./mvnw -B -ntp -pl shenyu-plugin/shenyu-plugin-ai/shenyu-plugin-ai-sensitive-word test -Dtest=SensitiveWordPluginTest18/18 green.
Checkstyle passes:
clean.
Notes
Cache semantics (
CachedDictionarywrite, staleness, fail-open/fail-closed priorities) are unchanged; only where the construction runs. The extraction also sets up the follow-up performance fix for redundant rule-word parsing (#7829), which touches the same method.