-
Notifications
You must be signed in to change notification settings - Fork 3k
Feat/improve #6271
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Feat/improve #6271
Conversation
- Optimize ShenyuWebHandler plugin chain execution to avoid stack overflow - Improve plugin management with O(1) lookup and CopyOnWrite strategy - Standardize concurrency control in ShenyuWebHandler - Simplify ShenyuPlugin API with PluginSkipHelper - Optimize logging performance with conditional execution - Add comprehensive tests for new functionality
- Modularize shenyu-bootstrap dependencies into profiles - Create 'standard' (default), 'minimal', 'rpc', 'logging', 'ai', 'mcp' profiles - Add documentation in docs/BOOTSTRAP_PROFILES.md
- Move nacos-discovery.version and eureka-client.version to parent pom properties - Remove redundant property definitions from shenyu-bootstrap/pom.xml
…in ShenyuWebHandler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request improves the ShenYu gateway by addressing plugin chain recursion issues, refactoring code for better maintainability, and reorganizing the build configuration. The changes focus on eliminating stack overflow risks with large numbers of skipped plugins, improving concurrency handling, and adding configurable plugin logging.
Changes:
- Converted plugin chain recursion to iteration to prevent stack overflow with many skipped plugins
- Added configurable plugin logging with enabled/minCost settings
- Refactored plugin skip logic into a dedicated PluginSkipHelper utility class
- Reorganized shenyu-bootstrap pom.xml into modular Maven profiles (standard, rpc, logging, ai, mcp)
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| shenyu-web/src/main/java/org/apache/shenyu/web/handler/ShenyuWebHandler.java | Refactored plugin chain to use iteration instead of recursion; improved thread-safety with copy-on-write pattern; added logging configuration |
| shenyu-web/src/test/java/org/apache/shenyu/web/handler/ShenyuWebHandlerTest.java | Enhanced tests for putExtPlugins and added concurrent modification test |
| shenyu-web/src/test/java/org/apache/shenyu/web/handler/ShenyuWebHandlerRecursionTest.java | New test file to verify handling of 10,000 skipped plugins without stack overflow |
| shenyu-plugin/shenyu-plugin-api/src/main/java/org/apache/shenyu/plugin/api/ShenyuPlugin.java | Refactored skip methods to delegate to PluginSkipHelper; added configurable logging to after() method |
| shenyu-plugin/shenyu-plugin-api/src/main/java/org/apache/shenyu/plugin/api/utils/PluginSkipHelper.java | New utility class extracting plugin skip logic for reusability |
| shenyu-plugin/shenyu-plugin-api/src/test/java/org/apache/shenyu/plugin/api/utils/PluginSkipHelperTest.java | New test file for PluginSkipHelper utility |
| shenyu-plugin/shenyu-plugin-api/src/test/java/org/apache/shenyu/plugin/api/ShenyuPluginTest.java | New test file for ShenyuPlugin default methods |
| shenyu-plugin/shenyu-plugin-api/src/main/java/org/apache/shenyu/plugin/api/exception/ShenyuPluginException.java | New exception class for plugin-specific errors |
| shenyu-common/src/main/java/org/apache/shenyu/common/constant/Constants.java | Added LOGGING_ENABLED and LOGGING_MIN_COST constants |
| shenyu-common/src/main/java/org/apache/shenyu/common/config/ShenyuConfig.java | Added PluginConfig class with enabled and minCost fields |
| shenyu-bootstrap/src/main/resources/application.yml | Added plugin configuration section with enabled and minCost settings |
| shenyu-sdk/shenyu-sdk-httpclient/src/main/java/org/apache/shenyu/sdk/httpclient/HttpShenyuSdkClient.java | Fixed property key by removing trailing space from "http.connectionRequestTimeOut " |
| shenyu-sdk/shenyu-sdk-httpclient/src/test/java/org/apache/shenyu/sdk/httpclient/HttpShenyuSdkClientTest.java | Added test properties for timeout configuration |
| shenyu-bootstrap/pom.xml | Restructured dependencies into Maven profiles for modular plugin inclusion |
| pom.xml | Moved nacos-discovery and eureka-client versions to parent pom properties |
Comments suppressed due to low confidence (1)
shenyu-web/src/main/java/org/apache/shenyu/web/handler/ShenyuWebHandler.java:344
- There is a potential issue with the plugin.after() call in the finally block. If plugin.before() throws an exception, the after() method will still be called even though before() never completed successfully. This could lead to inconsistent state or NullPointerException if after() expects state set by before().
try {
plugin.before(exchange);
return plugin.execute(exchange, this);
} finally {
plugin.after(exchange);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
shenyu-web/src/main/java/org/apache/shenyu/web/handler/ShenyuWebHandler.java
Outdated
Show resolved
Hide resolved
shenyu-plugin/shenyu-plugin-api/src/main/java/org/apache/shenyu/plugin/api/ShenyuPlugin.java
Outdated
Show resolved
Hide resolved
shenyu-web/src/main/java/org/apache/shenyu/web/handler/ShenyuWebHandler.java
Outdated
Show resolved
Hide resolved
...gin/shenyu-plugin-api/src/main/java/org/apache/shenyu/plugin/api/utils/PluginSkipHelper.java
Outdated
Show resolved
Hide resolved
shenyu-plugin/shenyu-plugin-api/src/main/java/org/apache/shenyu/plugin/api/ShenyuPlugin.java
Outdated
Show resolved
Hide resolved
shenyu-plugin/shenyu-plugin-api/src/main/java/org/apache/shenyu/plugin/api/ShenyuPlugin.java
Outdated
Show resolved
Hide resolved
| } else { | ||
| // Add new plugin | ||
| newPlugins.add(extPlugin); | ||
| this.sourcePlugins.add(extPlugin); |
Copilot
AI
Jan 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| this.sourcePlugins.add(extPlugin); | |
| synchronized (this.sourcePlugins) { | |
| this.sourcePlugins.add(extPlugin); | |
| } |
…ebHandler.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…u/plugin/api/ShenyuPlugin.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…u/plugin/api/ShenyuPlugin.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…u/plugin/api/ShenyuPlugin.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…u/plugin/api/utils/PluginSkipHelper.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…fer and change `before` execution timing, and add VS Code settings and e2e test logs.
Make sure that:
./mvnw clean install -Dmaven.javadoc.skip=true.