Repository navigation
Conversation
SelectorEventPublisher.onDeleted(Collection, List) resolved each selector's plugin name from a map built of the supplied plugins and called pluginName.equals(DIVIDE) on it. Callers do not guarantee the map covers every selector: SelectorServiceImpl#deleteByNamespaceId builds it from pluginMapper.selectByIds, which silently drops ids whose plugin row no longer exists, so deleting a dangling selector (one whose plugin was already removed) threw NullPointerException inside the event publisher and failed the whole deletion. The sibling RuleEventPublisher.onDeleted performs the same lookup null-safely via Optional. Compare constant-first, which keeps the divide branch intact and lets the deletion events publish with a null plugin name. The new test fails on current master with the exact NPE and passes with this change.
Aias00
left a comment
There was a problem hiding this comment.
The null-safe comparison prevents the admin-side NPE, but the resulting SelectorData still carries pluginName=null. Bootstrap deletion is keyed by plugin name: CommonPluginDataSubscriber, BaseDataCache, and MatchDataCache use selectorData.getPluginName() to remove selector, rule, and match-cache entries. A null-name delete therefore cannot reliably clear the stale data and may only move the failure downstream. Please preserve the plugin name for deletion or add a selector-id fallback in the data plane, with a regression test proving a missing-plugin selector delete actually removes bootstrap caches.
|
Confirmed the downstream behavior — you are right that the null name moves the problem:
On the two remedies: (a) preserve the plugin name — I could not find a source when the plugin row is already deleted: (b) selector-id fallback in the data plane — I sketched it: in How would you like to proceed?
Happy to implement either on your word — the regression test proving a blank-name selector delete purges |
Follow-up to the admin-side null-safe plugin lookup: a dangling selector's deletion event now reaches the bootstrap with a null pluginName, and every data-plane removal keyed by that name fails. ConcurrentHashMap rejects null keys, so BaseDataCache#removeSelectData's computeIfPresent throws NPE inside the subscriber, and the entry stays cached under the plugin name the selector had before its plugin row vanished. When the plugin name is blank, BaseDataCache#removeSelectData, MatchDataCache#removeSelectorData and #removeRuleDataBySelector now sweep every plugin bucket and drop entries with the matching selector id, #removeRuleData sweeps by rule id, and the empty-placeholder cleanups become no-ops. The subscriber skips the handler lookup instead of calling handlerMap.get(null), for rules as well because RuleEventPublisher already publishes null-name deletes null-safely. The regression tests cache a selector under its historical plugin name and prove a blank-name delete purges BaseDataCache and both MatchDataCache maps without touching other plugins' entries; all five fail on the previous commit (two assertion failures, three NPEs) and pass with this change.
|
Went with option 2 — the selector-id fallback is folded into this PR now (commit What the data plane does on a blank plugin name (null or empty,
Why the sweep targets the historical name's bucket correctly: a null-name selector can never have been cached ( Regression coverage (all five fail on the previous commit — two assertion failures and three NPEs — and pass with the fix):
Full |
Aias00
left a comment
There was a problem hiding this comment.
Re-review → approving
You took the selector-id fallback option, and the data plane now actually clears stale entries for a selector whose plugin row is gone — which was the whole point of the objection. Approving.
Admin side. SelectorEventPublisher:99 flips to PluginEnum.DIVIDE.getName().equals(pluginName), the constant-on-the-left form, so a null lookup no longer NPEs and the removeByKey branch is simply skipped. testOnDeletedSelectorReferencingMissingPlugin exercises onDeleted with an empty plugin list.
Data plane. Three places handled, which is the right set:
BaseDataCache.removeSelectData:51-64— blank plugin name now sweeps every plugin bucket by selector id (selectorMap.keySet().forEach(...)), delegating to the new privateremoveSelectData(pluginName, selectorId). This is the piece that makes the delete actually land: previouslycomputeIfPresent(null, ...)was a no-op (andConcurrentHashMapwould have thrown on a null key in the handler map), so the stale selector stayed cached forever.MatchDataCache—removeSelectorData,removeRuleDataandremoveRuleDataBySelectoreach gained the same blank → sweep-all-buckets path, andremoveEmptySelectorData/removeEmptyRuleDatanow short-circuit on blank instead of looking up a null key.CommonPluginDataSubscriber.removeCacheData— thehandlerMap.get(...)call is now guarded byStringUtils.isNotBlank, avoiding the null-key NPE on the concurrent map.
The tests prove the end-to-end property I asked for rather than just "does not throw": testUnSelectorSubscribeWithMissingPluginName caches a selector under its real plugin name, sends a deletion event with no plugin name, and asserts obtainSelectorData(mockPluginName1) and both MatchDataCache lookups are now null while the unrelated plugin keeps its entry. That is exactly the "bootstrap caches are really cleared" evidence. testRemoveSelectDataSweepsAllBucketsWhenPluginNameIsMissing and the two MatchDataCacheTest cases cover the fallback in isolation.
CI
Red checks are not from this change — every one of them fails while compiling shenyu-plugin-mcp-server:
[ERROR] .../ShenyuStreamableHttpServerTransportProvider.java:[841,24]
constructor MessageHandlingResult cannot be applied to given types;
required: int,java.lang.Object,java.lang.String,io.modelcontextprotocol.json.McpJsonMapper
found: int,io.modelcontextprotocol.spec.McpSchema.JSONRPCMessage,java.lang.String
Same failure appears on #7375 which touches only the logging plugin, so it is a master-level break from the MCP SDK gaining a constructor parameter. Please re-run once it is fixed; nothing here can be validated by these runs in the meantime.
Non-blocking
handler.removeSelector(...)is now skipped entirely for a dangling selector. That is correct for the NPE, but it means any plugin that releases resources inremoveSelector(connection pools, scheduled tasks, native handles) will not be invoked for this path — the cache is clean, the handler state is not. A fallback that resolves the plugin name from the cachedSelectorData(you already have it inBaseDataCache) and still calls the handler would close that gap. Worth a follow-up rather than blocking.- The sweep is O(plugins × paths) per dangling delete. Fine for a rare path, but if an operator deletes a plugin with many orphaned selectors in one burst this becomes a full-map scan per selector. Batching (collect ids, sweep once) would remove that; noting it because the same pattern is now in three methods.
removeEmptySelectorData/removeEmptyRuleDatareturning early on blank leaves empty per-plugin maps behind. Harmless memory-wise, but the "empty bucket" cleanup that those methods exist for is skipped precisely in the dangling case — the one case that creates empty buckets.- The sweeps iterate
ConcurrentHashMapviews while callingcomputeIfPresent/removeIf. That is legal (weakly consistent iterators, no CME), and the operations are idempotent, so a concurrent writer can only cause a missed entry that a later delete or restart clears. Just recording it so nobody "fixes" it by copying to a list and reintroducing a lock.
Found by code audit (no existing issue — happy to file one if maintainers prefer).
SelectorEventPublisher#onDeleted(Collection, List)resolves each selector's plugin name viapluginMap.get(selectorDO.getPluginId())and immediately callspluginName.equals(PluginEnum.DIVIDE.getName()). Callers do not guarantee the map covers every selector:SelectorServiceImpl#deleteByNamespaceIdbuilds it frompluginMapper.selectByIds(...), which silently drops ids whose plugin row no longer exists — so deleting a dangling selector (one whose plugin was already removed, reachable becauseselectByIdSetdoes not join plugins) throwsNullPointerExceptioninside the event publisher and fails the whole deletion request. The siblingRuleEventPublisher#onDeletedperforms the same plugin lookup null-safely viaOptional, confirming the anticipated state.Make sure that:
./mvnw test -pl shenyu-admin -am and ./mvnw checkstyle:check -pl shenyu-admin(module-scoped; full build left to CI).Modifications
PluginEnum.DIVIDE.getName().equals(pluginName)— null-safe, identical semantics for present plugin names, and the deletion events now publish with a null plugin name instead of crashing.Verifying this change
testOnDeletedSelectorReferencingMissingPlugindeletes a selector whose pluginId is absent from the plugins list and asserts both deletion events still publish. It fails on current master withNullPointerException: Cannot invoke "String.equals(Object)" because "pluginName" is nulland passes with this change.shenyu-adminmodule suite green (502 test classes); checkstyle green.Notes
SelectorEventPublisher(checked against the file lists of all 198 open PRs).