Copilot commented on code in PR #6271:
URL: https://github.com/apache/shenyu/pull/6271#discussion_r2689117458
##########
shenyu-web/src/main/java/org/apache/shenyu/web/handler/ShenyuWebHandler.java:
##########
@@ -147,47 +150,74 @@ public List<ShenyuPlugin> getPlugins() {
*
* @param extPlugins the ext plugins
*/
- public void putExtPlugins(final List<ShenyuPlugin> extPlugins) {
+ public synchronized void putExtPlugins(final List<ShenyuPlugin>
extPlugins) {
if (CollectionUtils.isEmpty(extPlugins)) {
return;
}
- final List<ShenyuPlugin> shenyuAddPlugins = extPlugins.stream()
- .filter(e -> plugins.stream().noneMatch(plugin ->
plugin.named().equals(e.named())))
- .collect(Collectors.toList());
- final List<ShenyuPlugin> shenyuUpdatePlugins = extPlugins.stream()
- .filter(e -> plugins.stream().anyMatch(plugin ->
plugin.named().equals(e.named())))
- .collect(Collectors.toList());
+ // Copy-on-write: create new list based on current volatile list
+ List<ShenyuPlugin> newPlugins = new ArrayList<>(this.plugins);
- if (CollectionUtils.isEmpty(shenyuAddPlugins) &&
CollectionUtils.isEmpty(shenyuUpdatePlugins)) {
- return;
+ // Create a map of existing plugins for O(1) lookup
+ Map<String, ShenyuPlugin> existingPluginMap = newPlugins.stream()
+ .collect(Collectors.toMap(ShenyuPlugin::named,
Function.identity()));
+
+ boolean hasAdditions = false;
+ boolean hasUpdates = false;
+
+ // Single pass through extPlugins to handle both additions and updates
+ for (ShenyuPlugin extPlugin : extPlugins) {
+ String pluginName = extPlugin.named();
+ if (existingPluginMap.containsKey(pluginName)) {
+ // Update existing plugin
+ updatePluginInList(extPlugin, newPlugins);
+ updatePluginInSource(extPlugin);
+ hasUpdates = true;
+ LOG.info("shenyu auto update extends plugins:{}", pluginName);
+ } else {
+ // Add new plugin
+ newPlugins.add(extPlugin);
+ this.sourcePlugins.add(extPlugin);
Review Comment:
The sourcePlugins list is accessed without synchronization in
putExtPlugins() at line 180. Since putExtPlugins() is synchronized, but
sourcePlugins is not a volatile or thread-safe collection, concurrent reads
from other methods could see inconsistent state during the add operation.
```suggestion
synchronized (this.sourcePlugins) {
this.sourcePlugins.add(extPlugin);
}
```
##########
shenyu-web/src/main/java/org/apache/shenyu/web/handler/ShenyuWebHandler.java:
##########
@@ -54,7 +56,7 @@ public final class ShenyuWebHandler implements WebHandler,
ApplicationListener<P
private static final Logger LOG =
LoggerFactory.getLogger(ShenyuWebHandler.class);
/**
- * this filed can not set to be final, because we should copyOnWrite to
update plugins.
+ * this field uses volatile List for thread-safe operations.
Review Comment:
The JavaDoc comment states "this field uses volatile List for thread-safe
operations" but this is misleading. The volatile keyword only ensures
visibility of the reference to the List, not thread-safe operations on the List
itself. The thread-safety is achieved through the copy-on-write pattern
combined with synchronized methods, not through volatile alone.
```suggestion
* Volatile reference to the current plugins list to ensure visibility
across threads.
* Thread safety of list updates is provided by the surrounding update
logic, not by volatile alone.
```
##########
shenyu-plugin/shenyu-plugin-api/src/main/java/org/apache/shenyu/plugin/api/ShenyuPlugin.java:
##########
@@ -129,7 +120,7 @@ default boolean skipExcept(ServerWebExchange exchange,
RpcTypeEnum... exceptRpcT
* @return http/spring cloud return true, others false.
Review Comment:
The JavaDoc comment for skipExceptHttpLike states "http/spring cloud return
true, others false" but the implementation returns false for
HTTP/SPRING_CLOUD/AI (meaning they should NOT be skipped) and true for others
(meaning they SHOULD be skipped). The documentation should be corrected to say
"http/spring cloud/ai return false (not skipped), others return true (skipped)".
```suggestion
* @return http/spring cloud/ai return false (not skipped), others
return true (skipped).
```
##########
shenyu-plugin/shenyu-plugin-api/src/main/java/org/apache/shenyu/plugin/api/utils/PluginSkipHelper.java:
##########
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shenyu.plugin.api.utils;
+
+import org.apache.commons.lang3.ArrayUtils;
+import org.apache.shenyu.common.constant.Constants;
+import org.apache.shenyu.common.enums.RpcTypeEnum;
+import org.apache.shenyu.plugin.api.context.ShenyuContext;
+import org.springframework.web.server.ServerWebExchange;
+
+import java.util.Arrays;
+import java.util.Objects;
+
+/**
+ * Plugin skip helper.
+ */
+public final class PluginSkipHelper {
+
+ private PluginSkipHelper() {
+ }
+
+ /**
+ * plugin is executed.
+ * if return true this plugin can not execute.
+ *
+ * <p>the same for:
+ * <pre>
+ * <code>Objects.equals(rpcType, typeA.getName())
+ * || Objects.equals(rpcType, typeB.getName())
+ * || Objects.equals(rpcType, type...getName())
+ * </code>
+ * </pre>
+ *
+ * @param exchange the current server exchange
+ * @param rpcTypes the skip rpc type list
+ * @return current rpcType == someone rpcType
+ */
+ public static boolean skip(final ServerWebExchange exchange, final
RpcTypeEnum... rpcTypes) {
+ if (ArrayUtils.isEmpty(rpcTypes)) {
+ return false;
+ }
+ ShenyuContext shenyuContext = exchange.getAttribute(Constants.CONTEXT);
+ Objects.requireNonNull(shenyuContext);
+ return Arrays.stream(rpcTypes).anyMatch(type ->
Objects.equals(shenyuContext.getRpcType(), type.getName()));
+ }
+
+ /**
+ * the plugin execute skip except some rpc types.
+ * if return true this plugin can not execute.
+ *
+ * <p>the same for:
+ * <pre>
+ * <code>!Objects.equals(rpcType, typeA.getName())
+ * && !Objects.equals(rpcType, typeB.getName())
+ * && !Objects.equals(rpcType, type...getName())
+ * </code>
+ * </pre>
+ *
+ * @param exchange the current server exchange
+ * @param exceptRpcTypes the except rpc type list
+ * @return current rpcType != someone exceptRpcType
+ */
+ public static boolean skipExcept(final ServerWebExchange exchange, final
RpcTypeEnum... exceptRpcTypes) {
+ return !skip(exchange, exceptRpcTypes);
+ }
+
+ /**
+ * Skip the non http call.
+ * if return true this plugin can not execute.
+ *
+ * @param exchange the current server exchange
+ * @return http/spring cloud return true, others false.
Review Comment:
The JavaDoc comment for skipExceptHttpLike states "http/spring cloud return
true, others false" but the implementation returns false for
HTTP/SPRING_CLOUD/AI (meaning they should NOT be skipped) and true for others
(meaning they SHOULD be skipped). The documentation should be corrected to say
"http/spring cloud/ai return false (not skipped), others return true (skipped)".
```suggestion
* @return http/spring cloud/ai return false (not skipped), others
return true (skipped).
```
##########
shenyu-plugin/shenyu-plugin-api/src/main/java/org/apache/shenyu/plugin/api/ShenyuPlugin.java:
##########
@@ -147,9 +138,17 @@ default void before(ServerWebExchange exchange) {
* @param exchange context
*/
default void after(ServerWebExchange exchange) {
+ Boolean loggingEnabled =
exchange.getAttributeOrDefault(Constants.LOGGING_ENABLED, true);
+ if (!loggingEnabled) {
+ return;
+ }
long currentTimeMillis = System.currentTimeMillis();
long startTime = (long)
exchange.getAttributes().get(Constants.PLUGIN_START_TIME + named());
Review Comment:
In the after() method, there's a potential NullPointerException if the
PLUGIN_START_TIME attribute was never set or was removed. The code assumes the
attribute exists when calling exchange.getAttributes().get(), which could
return null and cause an unboxing error when casting to long.
```suggestion
Long startTime = exchange.getAttribute(Constants.PLUGIN_START_TIME +
named());
if (startTime == null) {
exchange.getAttributes().remove(Constants.PLUGIN_START_TIME +
named());
return;
}
```
##########
shenyu-plugin/shenyu-plugin-api/src/main/java/org/apache/shenyu/plugin/api/ShenyuPlugin.java:
##########
@@ -147,9 +138,17 @@ default void before(ServerWebExchange exchange) {
* @param exchange context
*/
default void after(ServerWebExchange exchange) {
+ Boolean loggingEnabled =
exchange.getAttributeOrDefault(Constants.LOGGING_ENABLED, true);
+ if (!loggingEnabled) {
+ return;
+ }
long currentTimeMillis = System.currentTimeMillis();
long startTime = (long)
exchange.getAttributes().get(Constants.PLUGIN_START_TIME + named());
- LOG.debug("shenyu traceId:{}, plugin named:{}, cost:{}",
exchange.getLogPrefix(), named(), currentTimeMillis - startTime);
+ long cost = currentTimeMillis - startTime;
+ Long minCost =
exchange.getAttributeOrDefault(Constants.LOGGING_MIN_COST, 0L);
+ if (cost >= minCost) {
+ LOG.info("shenyu traceId:{}, plugin named:{}, cost:{}",
exchange.getLogPrefix(), named(), cost);
Review Comment:
The logging level has been changed from LOG.debug to LOG.info. This could
result in significantly increased log volume in production environments,
potentially impacting performance and log storage. Consider making this
configurable or keeping it at debug level for per-plugin timing information.
```suggestion
LOG.debug("shenyu traceId:{}, plugin named:{}, cost:{}",
exchange.getLogPrefix(), named(), cost);
```
##########
shenyu-web/src/main/java/org/apache/shenyu/web/handler/ShenyuWebHandler.java:
##########
@@ -237,39 +271,48 @@ private List<ShenyuPlugin> sortPlugins(final
List<ShenyuPlugin> list) {
*
* @param pluginData plugin data
*/
- private synchronized void onPluginEnabled(final PluginData pluginData) {
+ private void onPluginEnabled(final PluginData pluginData) {
LOG.info("shenyu use plugin:[{}]", pluginData.getName());
if (StringUtils.isNoneBlank(pluginData.getPluginJar())) {
LOG.info("shenyu start load plugin [{}] from upload plugin jar",
pluginData.getName());
shenyuLoaderService.loadExtOrUploadPlugins(pluginData);
}
final List<ShenyuPlugin> enabledPlugins =
this.sourcePlugins.stream().filter(plugin ->
plugin.named().equals(pluginData.getName())
&& pluginData.getEnabled()).collect(Collectors.toList());
- enabledPlugins.removeAll(this.plugins);
- // copy a new plugin list.
- List<ShenyuPlugin> newPluginList = new ArrayList<>(this.plugins);
- newPluginList.addAll(enabledPlugins);
- this.plugins = sortPlugins(newPluginList);
+
+ // Copy-on-write
+ List<ShenyuPlugin> newPlugins = new ArrayList<>(this.plugins);
+ enabledPlugins.removeAll(newPlugins);
+ if (!enabledPlugins.isEmpty()) {
+ // Add enabled plugins
+ newPlugins.addAll(enabledPlugins);
+ // Re-sort the plugins list
+ List<ShenyuPlugin> sortedPlugins = sortPlugins(newPlugins);
+ // Volatile write
+ this.plugins = sortedPlugins;
+ }
}
/**
* handle removed or disabled plugin.
*
* @param pluginData plugin data
*/
- private synchronized void onPluginRemoved(final PluginData pluginData) {
- // copy a new plugin list.
- List<ShenyuPlugin> newPluginList = new ArrayList<>(this.plugins);
- newPluginList.removeIf(plugin ->
plugin.named().equals(pluginData.getName()));
- this.plugins = newPluginList;
+ private void onPluginRemoved(final PluginData pluginData) {
+ // Copy-on-write
+ List<ShenyuPlugin> newPlugins = new ArrayList<>(this.plugins);
+ // Remove plugin from plugins list
+ newPlugins.removeIf(plugin ->
plugin.named().equals(pluginData.getName()));
+ // Volatile write
+ this.plugins = newPlugins;
}
private static class DefaultShenyuPluginChain implements ShenyuPluginChain
{
- private int index;
+ private final AtomicInteger index = new AtomicInteger(0);
Review Comment:
The AtomicInteger index in DefaultShenyuPluginChain is shared across
multiple invocations of execute(). Since a new DefaultShenyuPluginChain
instance is created for each request in the handle() method, each request will
have its own independent index. However, the use of AtomicInteger here is
unnecessary overhead since each chain instance is not shared between threads
for the same request. A simple int field would be more efficient.
--
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]
