Copilot commented on code in PR #7035:
URL: https://github.com/apache/shenyu/pull/7035#discussion_r4034739325
##########
shenyu-plugin/shenyu-plugin-base/src/main/java/org/apache/shenyu/plugin/base/cache/BaseDataCache.java:
##########
@@ -287,13 +291,65 @@ private void ruleAccept(final RuleData data) {
* @param data the selector data
*/
private void selectorAccept(final SelectorData data) {
+ selectorAccept(selectorMap, data);
+ }
+
+ private void selectorAccept(final ConcurrentMap<String,
List<SelectorData>> target, final SelectorData data) {
String key = data.getPluginName();
- SELECTOR_MAP.compute(key, (pluginName, value) -> {
+ target.compute(key, (pluginName, value) -> {
final List<SelectorData> result = Objects.isNull(value) ? new
ArrayList<>() : new ArrayList<>(value);
result.removeIf(selector -> Objects.equals(selector.getId(),
data.getId()));
result.add(data);
result.sort(Comparator.comparing(SelectorData::getSort));
return List.copyOf(result);
});
}
+
+ /**
+ * Merge a batch without exposing partially refreshed data to readers.
+ * Missing entries are retained because refresh messages may cover only
one plugin.
+ *
+ * @param dataList the received data
+ */
+ void refreshPluginData(final List<PluginData> dataList) {
+ if (dataList.isEmpty()) {
+ return;
+ }
+ ConcurrentMap<String, PluginData> next = Maps.newConcurrentMap();
+ next.putAll(pluginMap);
+ dataList.forEach(data -> next.put(data.getName(), data));
+ pluginMap = next;
Review Comment:
This copy-and-swap can lose a concurrent plugin update. If `cachePluginData`
(or a remove/clean operation) mutates the current map after `putAll` but before
this assignment, the mutation stays on the old map and disappears when `next`
is published. Multiple WebSocket clients share this subscriber, so refresh and
push callbacks can overlap. Please coordinate every plugin-map writer with
refresh (or use a CAS/retry snapshot design) and add a concurrent
refresh/update regression test.
This issue also appears in the following locations of the same file:
- line 334
- line 350
##########
shenyu-plugin/shenyu-plugin-base/src/main/java/org/apache/shenyu/plugin/base/cache/CommonPluginDataSubscriber.java:
##########
@@ -310,4 +281,83 @@ private <T> void removeCacheData(@NonNull final T data) {
}
}
+ private void notifyPluginData(final PluginData oldPluginData, final
PluginData pluginData) {
+ // update enabled plugins
+ PluginHandlerEventEnum state =
Boolean.TRUE.equals(pluginData.getEnabled())
+ ? PluginHandlerEventEnum.ENABLED :
PluginHandlerEventEnum.DISABLED;
+ eventPublisher.publishEvent(new PluginHandlerEvent(state, pluginData));
+ // sorted plugin
+ sortPluginIfOrderChange(oldPluginData, pluginData);
+
+ final String pluginName = pluginData.getName();
+ // if update plugin, remove selector and rule match cache/trie cache
+ if (selectorMatchConfig.getCache().getEnabled()) {
+ MatchDataCache.getInstance().removeSelectorData(pluginName);
+ }
+ if (ruleMatchCacheConfig.getCache().getEnabled()) {
+ MatchDataCache.getInstance().removeRuleData(pluginName);
+ }
+ }
+
+ private void handleSelectorData(final SelectorData selectorData) {
+ Optional.ofNullable(handlerMap.get(selectorData.getPluginName()))
+ .ifPresent(handler -> handler.handlerSelector(selectorData));
+ invalidateSelectorMatchCache(selectorData);
+ }
+
+ private void invalidateSelectorMatchCache(final SelectorData selectorData)
{
+ // remove match cache
+ if (selectorMatchConfig.getCache().getEnabled()) {
+
MatchDataCache.getInstance().removeSelectorData(selectorData.getPluginName(),
selectorData.getId());
+
MatchDataCache.getInstance().removeEmptySelectorData(selectorData.getPluginName());
+ }
+ if (ruleMatchCacheConfig.getCache().getEnabled()) {
+
MatchDataCache.getInstance().removeRuleDataBySelector(selectorData.getPluginName(),
selectorData.getId());
+
MatchDataCache.getInstance().removeEmptyRuleData(selectorData.getPluginName());
+ }
+ }
+
+ private void handleRuleData(final RuleData ruleData) {
+ Optional.ofNullable(handlerMap.get(ruleData.getPluginName()))
+ .ifPresent(handler -> handler.handlerRule(ruleData));
+ invalidateRuleMatchCache(ruleData);
+ }
+
+ private void invalidateRuleMatchCache(final RuleData ruleData) {
+ if (ruleMatchCacheConfig.getCache().getEnabled()) {
+
MatchDataCache.getInstance().removeRuleData(ruleData.getPluginName(),
ruleData.getId());
+
MatchDataCache.getInstance().removeEmptyRuleData(ruleData.getPluginName());
+ }
+ }
+
+ @Override
+ public void onPluginRefresh(final List<PluginData> dataList) {
+ if (CollectionUtils.isEmpty(dataList)) {
+ return;
+ }
+ dataList.forEach(data ->
Optional.ofNullable(handlerMap.get(data.getName()))
+ .ifPresent(handler -> handler.handlerPlugin(data)));
+ BaseDataCache.getInstance().refreshPluginData(dataList);
Review Comment:
A plugin handler exception now aborts the whole batch before
`refreshPluginData` publishes it. The previous refresh path called
`onSubscribe` per item, whose `subscribeDataHandler` catches each exception, so
later entries were still processed. Preserve that per-entry exception isolation
here; otherwise one malformed plugin config prevents every valid plugin in the
refresh from being applied.
This issue also appears in the following locations of the same file:
- line 350
- line 359
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
