Conversation
这里存在线程不安全的情况,改成ConcurrentHashMap
|
|
significantfrank
left a comment
There was a problem hiding this comment.
PR #569 代码审查报告
变更摘要: 将 ExtensionRepository.extensionRepo 从 HashMap 改为 ConcurrentHashMap,以解决线程安全问题。
审查结论:🟠 需修改
此 PR 的意图正确(识别到 HashMap 在并发场景下的不安全),但仅替换实现类不够,还需同步修改写入方式才能真正保证线程安全。
发现汇总
| 等级 | 数量 | 说明 |
|---|---|---|
| 🔴 Critical | 0 | — |
| 🟠 Major | 1 | ConcurrentHashMap + put 的 check-then-act 仍非原子,应使用 putIfAbsent |
| 🟡 Minor | 1 | ConcurrentHashMap 不允许 null value,需确认注册路径无 null |
| 🔵 Suggestion | 1 | getExtensionRepo() 暴露可变 Map,建议封装 |
核心问题
ExtensionRegister 中的写入模式为:
ExtensionPointI preVal = extensionRepository.getExtensionRepo().put(key, value);
if (preVal != null) { throw ...; }这是典型的 check-then-act:先 put 再检查。ConcurrentHashMap.put() 虽然单次操作线程安全,但"put + 检查返回值"这一组合操作并非原子。两个线程可能同时 put 成功并都看到 preVal == null,导致重复注册漏检。
建议修改 ExtensionRegister,使用 putIfAbsent:
ExtensionPointI preVal = extensionRepository.getExtensionRepo().putIfAbsent(key, value);
if (preVal != null) { throw ...; }补充说明
若扩展注册仅在 Spring 容器初始化阶段(@PostConstruct)单线程执行,之后运行时只读,则原始 HashMap 已足够安全,改 ConcurrentHashMap 反而增加不必要复杂度。建议 PR 作者先确认实际并发场景再决定修改方案。
| } | ||
|
|
||
| private Map<ExtensionCoordinate, ExtensionPointI> extensionRepo = new HashMap<>(); | ||
| private Map<ExtensionCoordinate, ExtensionPointI> extensionRepo = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
🟠 Major: 仅将 HashMap 替换为 ConcurrentHashMap 并不能保证线程安全。ExtensionRegister 中的写入路径是 check-then-act 模式:先 put 再检查 preVal != null 来检测重复,但 ConcurrentHashMap.put() 本身不是原子的 check-then-act 操作——两个线程可能同时 put 成功并都看到 preVal == null,导致重复注册被漏检。
建议方案:
- 使用
ConcurrentHashMap+putIfAbsent替代put+ 检查返回值,在ExtensionRegister中改为:ExtensionPointI preVal = extensionRepository.getExtensionRepo().putIfAbsent(extensionCoordinate, extensionObject); - 或者,若注册只在 Spring
@PostConstruct阶段单线程执行,运行时只读,则HashMap已足够,加volatile或改ConcurrentHashMap反而增加不必要的复杂度。请确认实际并发场景。
| } | ||
|
|
||
| private Map<ExtensionCoordinate, ExtensionPointI> extensionRepo = new HashMap<>(); | ||
| private Map<ExtensionCoordinate, ExtensionPointI> extensionRepo = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
🟡 Minor: ConcurrentHashMap 不允许 null key 或 null value,而 HashMap 允许。当前 ExtensionCoordinate 作为 key 应该不会为 null,但请确认所有注册路径不会传入 null value,否则运行时将从静默接受变为抛出 NullPointerException。
| } | ||
|
|
||
| private Map<ExtensionCoordinate, ExtensionPointI> extensionRepo = new HashMap<>(); | ||
| private Map<ExtensionCoordinate, ExtensionPointI> extensionRepo = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
🔵 Suggestion: getExtensionRepo() 直接暴露内部可变 Map 的引用,外部代码可以直接修改 Map 内容,破坏封装性。建议:
- 若运行时只读,返回
Collections.unmodifiableMap(extensionRepo) - 或提供
get(ExtensionCoordinate)方法,避免暴露整个 Map - 若确实需要外部写入(如
ExtensionRegister),考虑提供专门的register/putIfAbsent方法
这里存在线程不安全的情况,改成ConcurrentHashMap