Skip to content

fix(ai): build the sensitive-word dictionary off the redis io threads (#7830) - #7844

Closed
konlue wants to merge 2 commits into
apache:masterfrom
konlue:fix/sensitive-word-io-thread
Closed

konlue wants to merge 2 commits into
apache:masterfrom
konlue:fix/sensitive-word-io-thread

Conversation

@konlue

@konlue konlue commented Oct 19, 2026

Copy link
Copy Markdown
Contributor

Fixes #7830.

Summary

SensitiveWordPlugin#dictionary relied on subscribeOn(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. The onErrorResume fallback built the rule-words automaton inline on the error thread as well.

Modifications

  • Hop with publishOn(Schedulers.boundedElastic()) between collectList() and the automaton construction.
  • Defer the fallback rule-words build with Mono.fromCallable + subscribeOn, so it never runs on the error-emitting thread.
  • Extract the two construction paths as package-private seams (redisAutomaton, ruleWordsAutomaton) to make the threading directly testable.

Verifying this change

  • testTheDictionaryIsBuiltOffTheRedisIoThreads: the redis source is pinned to a named simulated-redis-io scheduler; 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 SensitiveWordPluginTest 18/18, no regressions.

  • Targeted tests pass:

    ./mvnw -B -ntp -pl shenyu-plugin/shenyu-plugin-ai/shenyu-plugin-ai-sensitive-word test -Dtest=SensitiveWordPluginTest

    18/18 green.

  • Checkstyle passes:

    ./mvnw -B -ntp -pl shenyu-plugin/shenyu-plugin-ai/shenyu-plugin-ai-sensitive-word checkstyle:check

    clean.

Notes

Cache semantics (CachedDictionary write, 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.

Aias00
Aias00 previously approved these changes Oct 19, 2026

@Aias00 Aias00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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:

  • testTheDictionaryIsBuiltOffTheRedisIoThreads drives the source on a dedicated simulated-redis-io parallel scheduler and asserts the build thread name does not start with that prefix. Without publishOn this test fails, because the map would run on the emitting thread — so it pins the actual defect.
  • testTheRuleWordsAutomatonBuildIsDeferred asserts that assembling ruleWordsAutomaton(...) does not invoke AhoCorasick.of at all, then verifies the deferred build still works on subscription. That is precisely the eager-evaluation bug.

Three notes, none blocking:

  1. The fallback Mono is cold and rebuilds per subscription. Mono.fromCallable re-executes on every subscribe where the old Mono.just reused one instance. In the current flow dictionary() 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 calls cachedHandle). If the chain is ever retried or resubscribed, the automaton would be rebuilt each time off-loop. Worth a comment saying that is expected.

  2. Two scheduler hops. subscribeOn(boundedElastic) plus publishOn(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 the subscribeOn is arguably redundant now that publishOn guards the CPU stage. Not a correctness issue — just a hop worth dropping if this path ever shows up in profiles.

  3. Visibility. redisAutomaton and ruleWordsAutomaton are package-private only so the test can reach them. private plus @VisibleForTesting, or exercising them through dictionary()/doExecute, would keep the plugin’s surface tighter. Minor.

CI has not reported on this branch yet — please confirm the workflow passes before merge.

@konlue

konlue commented Oct 19, 2026

Copy link
Copy Markdown
Contributor Author

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.

@Aias00

Aias00 commented Oct 20, 2026

Copy link
Copy Markdown
Contributor

@konlue pls fix ci

@konlue

konlue commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

@Aias00 Done. Merged latest master to resolve the merge conflicts — the conflicts were against the already-merged #7850, took master's side, and SensitiveWordPluginTest is 20/20 green on the updated head. CI should go green now.

@Aias00

Aias00 commented Oct 21, 2026

Copy link
Copy Markdown
Contributor

Heads-up: this PR currently contains no changes relative to master.

GET /repos/apache/shenyu/pulls/7844  ->  changed_files: 0, additions: 0, deletions: 0
GET /repos/apache/shenyu/compare/261db07c...bb0941cb  ->  ahead_by: 2, files: 0

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 master by another route. There is nothing left to review or merge here.

Could you confirm that the fix you intended is the one now on master (in SensitiveWordPluginDataHandler) and close this PR? If you were targeting a different code path that still slurps Redis on the IO threads, point me at the file and I'll take another look — but as it stands the diff is empty.

@konlue

konlue commented Oct 21, 2026

Copy link
Copy Markdown
Contributor Author

Confirmed — that's correct. The dictionary-threading fix reached master via #7850: that PR was stacked on this branch (both changes touch the same method), and since it merged first, this PR's diff shrank to nothing. All four sensitive-word tests from the two PRs ( testTheDictionaryIsBuiltOffTheRedisIoThreads , testTheRuleWordsAutomatonBuildIsDeferred , and the two cache-parse spy tests) are on master now. Closing as merged-by-another-route — thanks for the careful check!

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] Sensitive-word dictionary construction can still run on the Redis I/O thread

2 participants