Skip to content

fix(core): fix check-then-act race conditions on ConcurrentHashMap in core module#14752

Closed
daguimu wants to merge 1 commit intoalibaba:developfrom
daguimu:fix/core-concurrent-map-race-conditions
Closed

fix(core): fix check-then-act race conditions on ConcurrentHashMap in core module#14752
daguimu wants to merge 1 commit intoalibaba:developfrom
daguimu:fix/core-concurrent-map-race-conditions

Conversation

@daguimu
Copy link
Copy Markdown
Contributor

@daguimu daguimu commented Mar 26, 2026

Problem

Two classes in the core module use containsKey() followed by get() on ConcurrentHashMap fields. Although each method is individually thread-safe, the compound operation is not atomic, creating a check-then-act race condition that can cause NullPointerException.

Race Condition Explained

Take IdGeneratorManager.nextId() as an example:

// Before (buggy)
if (generatorMap.containsKey(resource)) {       // Step 1: check key exists
    return generatorMap.get(resource).nextId();  // Step 2: get value and call method
}

Between Step 1 and Step 2, another thread may modify the map:

Thread A: containsKey("order") -> true
                                          Thread B: generatorMap.remove("order")
Thread A: generatorMap.get("order") -> null
Thread A: null.nextId() -> NullPointerException!

ConcurrentHashMap guarantees thread-safety for individual operations, but NOT for compound operations spanning two separate method calls. The time window between containsKey() returning true and the subsequent get() call is a race condition window.

Root Cause

  • IdGeneratorManager.generatorMap is a ConcurrentHashMap<String, IdGenerator> (line 38), populated via register() and accessed from multiple threads in nextId().
  • PluginManager.pluginRegistry is a ConcurrentHashMap<String, PluginInfo> (line 92), modified at runtime by ApplicationReadyEvent listener and plugin state synchronizer, while loadPersistedData() reads it.

Fix

Replace the two-step containsKey() + get() with a single get() call and null check:

// After (safe)
IdGenerator generator = generatorMap.get(resource);  // single atomic lookup
if (generator != null) {                              // null check on local variable
    return generator.nextId();                        // safe - local ref cannot be invalidated
}

This eliminates the race window because:

  1. Only one map access is performed (single get())
  2. The result is stored in a local variable, which cannot be affected by other threads
  3. The null check and method call operate on the local reference, not the map

Changes

Class Method Fix Points
IdGeneratorManager nextId(String resource) 1
PluginManager loadPersistedData() 2 (states loop + configs loop)

Tests

All existing tests pass (7 IdGeneratorManagerTest + 29 PluginManagerTest). The fix is behavior-preserving under non-concurrent conditions - it only prevents NPE when concurrent map modifications occur.

Impact

Prevents potential NPE in ID generation and plugin management paths. No behavioral change under normal single-threaded access.

… core module

Replace containsKey()+get() with single get()+null check in 2 classes:
- IdGeneratorManager.nextId
- PluginManager.loadPersistedData
@github-actions
Copy link
Copy Markdown

Thanks for your this PR. 🙏
Please check again for your PR changes whether contains any usage/api/configuration change such as Add new API , Add new configuration, Change default value of configuration.
If so, please add or update documents(markdown type) in docs/next/ for repository nacos-group/nacos-group.github.io


感谢您提交的PR。 🙏
请再次查看您的PR内容,确认是否包含任何使用方式/API/配置参数的变更,如:新增API新增配置参数修改默认配置等操作。
如果是,请确保在提交之前,在仓库nacos-group/nacos-group.github.io中的docs/next/目录下添加或更新文档(markdown格式)。

Copy link
Copy Markdown
Collaborator

@KomachiSion KomachiSion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR I think is over protected. So I think can't be merged.

Map<String, Boolean> states = persistence.loadAllStates();
states.forEach((pluginId, enabled) -> {
if (pluginRegistry.containsKey(pluginId)) {
PluginInfo pluginInfo = pluginRegistry.get(pluginId);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method will be called in single thread(only in once at ApplicationReadyEvent callback).

public long nextId(String resource) {
if (generatorMap.containsKey(resource)) {
return generatorMap.get(resource).nextId();
IdGenerator generator = generatorMap.get(resource);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These is no problem, IdGenerator will not be removed

@KomachiSion KomachiSion closed this Apr 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants