fix: ActionHandler.action线程安全问题#325
Merged
Merged
Conversation
Reviewer's Guide (审查者指南)Addressed thread safety in ActionHandler and PayloadSender by replacing the non-atomic echo counter with AtomicInteger and unifying the PayloadSender lock into a static field. (通过将非原子回显计数器替换为 AtomicInteger 并将 PayloadSender 锁统一到静态字段中，解决了 ActionHandler 和 PayloadSender 中的线程安全问题。) ER diagram for ActionHandler echo field update (ActionHandler 回显字段更新的 ER 图)erDiagram
ACTION_HANDLER {
INT echo
}
ACTION_HANDLER ||--o{ PAYLOAD : uses
PAYLOAD {
INT echo
}
Class diagram for thread safety changes in ActionHandler and PayloadSender (ActionHandler 和 PayloadSender 中线程安全更改的类图)classDiagram
class ActionHandler {
- AtomicInteger echo
+ ActionHandler(WebSocketProperties wsProp, RateLimiterProperties rateLimiterProps, RateLimiter rateLimiter)
...
}
class PayloadSender {
- static Lock lock
- Condition condition
...
}
File-Level Changes (文件级别更改)
Tips and commands (提示和命令)Interacting with Sourcery (与 Sourcery 交互)
Customizing Your Experience (自定义您的体验)Access your dashboard to: (访问您的 仪表板 以：)
Getting Help (获取帮助)
Original review guide in EnglishReviewer's GuideAddressed thread safety in ActionHandler and PayloadSender by replacing the non-atomic echo counter with AtomicInteger and unifying the PayloadSender lock into a static field. ER diagram for ActionHandler echo field updateerDiagram
ACTION_HANDLER {
INT echo
}
ACTION_HANDLER ||--o{ PAYLOAD : uses
PAYLOAD {
INT echo
}
Class diagram for thread safety changes in ActionHandler and PayloadSenderclassDiagram
class ActionHandler {
- AtomicInteger echo
+ ActionHandler(WebSocketProperties wsProp, RateLimiterProperties rateLimiterProps, RateLimiter rateLimiter)
...
}
class PayloadSender {
- static Lock lock
- Condition condition
...
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
嘿 @Chowhound-ch - 我已经查看了你的更改，它们看起来很棒！
AI 代理的提示
请解决此代码审查中的评论：
## 单独评论
### 评论 1
<location> `src/main/java/com/mikuac/shiro/handler/ActionHandler.java:157` </location>
<code_context>
JSONObject payload = new JSONObject();
payload.put("action", action.getPath());
- payload.put("echo", echo++);
+ payload.put("echo", echo.getAndAdd(1));
if (params != null && !params.isEmpty()) {
payload.put("params", params);
</code_context>
<issue_to_address>
考虑使用 getAndIncrement() 以提高清晰度。
getAndIncrement() 更加符合习惯用法，并且在递增 1 时提高了可读性。
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
payload.put("echo", echo.getAndAdd(1));
=======
payload.put("echo", echo.getAndIncrement());
>>>>>>> REPLACE
</suggested_fix>
### 评论 2
<location> `src/main/java/com/mikuac/shiro/common/utils/PayloadSender.java:36` </location>
<code_context>
private JSONObject resp;
- private final Lock lock = new ReentrantLock();
+ private static final Lock lock = new ReentrantLock();
private final Condition condition = lock.newCondition();
</code_context>
<issue_to_address>
将锁设为静态可能会在实例之间引入并发问题。
静态锁导致所有 PayloadSender 实例共享同一把锁，从而增加了处理多个会话时的争用和死锁风险。如果锁保护特定于实例的状态，请保持锁为非静态。
</issue_to_address>帮助我变得更有用！请点击每个评论上的 👍 或 👎，我将使用反馈来改进你的评论。
Original comment in English
Hey @Chowhound-ch - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/main/java/com/mikuac/shiro/handler/ActionHandler.java:157` </location>
<code_context>
JSONObject payload = new JSONObject();
payload.put("action", action.getPath());
- payload.put("echo", echo++);
+ payload.put("echo", echo.getAndAdd(1));
if (params != null && !params.isEmpty()) {
payload.put("params", params);
</code_context>
<issue_to_address>
Consider using getAndIncrement() for clarity.
getAndIncrement() is more idiomatic and improves readability when incrementing by one.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
payload.put("echo", echo.getAndAdd(1));
=======
payload.put("echo", echo.getAndIncrement());
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `src/main/java/com/mikuac/shiro/common/utils/PayloadSender.java:36` </location>
<code_context>
private JSONObject resp;
- private final Lock lock = new ReentrantLock();
+ private static final Lock lock = new ReentrantLock();
private final Condition condition = lock.newCondition();
</code_context>
<issue_to_address>
Making lock static may introduce concurrency issues across instances.
A static lock causes all PayloadSender instances to share the same lock, increasing contention and risk of deadlocks when handling multiple sessions. Keep the lock non-static if it protects instance-specific state.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| private JSONObject resp; | ||
|
|
||
| private final Lock lock = new ReentrantLock(); |
There was a problem hiding this comment.
issue (bug_risk): 将锁设为静态可能会在实例之间引入并发问题。
静态锁导致所有 PayloadSender 实例共享同一把锁，从而增加了处理多个会话时的争用和死锁风险。如果锁保护特定于实例的状态，请保持锁为非静态。
Original comment in English
issue (bug_risk): Making lock static may introduce concurrency issues across instances.
A static lock causes all PayloadSender instances to share the same lock, increasing contention and risk of deadlocks when handling multiple sessions. Keep the lock non-static if it protects instance-specific state.
Contributor
Author
|
又补了一点，ActionHandler.callback为HashMap，HashMap线程不安全，而并发下会在ActionHandler.onReceiveActionResp方法大量get，会导致大量并发下最后几个请求没法被condition.signalAll()唤醒（20次不会，200次则可能最后几个会出问题），换成ConcurrentHashMap则不会出现。 |
提高代码可读性 Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Owner
|
LGTM, Thanks! |
Chowhound-ch
added a commit
to Chowhound-ch/Chowh-Bot-Shiro
that referenced
this pull request
Jul 18, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ActionHandler.action多线程下会出现线程安全问题，返回错误响应retCode=-1。
原因：
效果：
修改前：
修改后：
测试代码：