[Bug] Thread leak in NacosRegistry.scheduleServiceNamesLookup due to race condition
- Dominant language
- Java
- Stars
- 41.6k
- Forks
- 26.4k
- Avg merge
- 15h 13m
- Merged PRs (30d)
- 4
Description
### Pre-check
- [x] I am sure that all the content I provide is in English.
### Search before asking
- [x] I had searched in the [issues](https://github.com/apache/dubbo/issues?q=is%3Aissue) and found no similar issues.
### Apache Dubbo Component
Java SDK (apache/dubbo)
### Dubbo Version
Dubbo 3.3.x (Confirmed in latest master branch) & Dubbo 2.7.x.
JDK 1.8+, All Operating Systems.
### Steps to reproduce this issue
1. This issue is identified via static code analysis (concurrency safety check).
2. It is located in `org.apache.dubbo.registry.nacos.NacosRegistry` method `scheduleServiceNamesLookup`.
3. The method uses a "Check-Then-Act" pattern to lazily initialize the `scheduledExecutorService`.
4. Although the field is `volatile`, the initialization block is NOT synchronized.
Code Analysis:
When `scheduleServiceNamesLookup` is called concurrently (e.g. via admin protocol), a race condition occurs:
if (scheduledExecutorService == null) { // Thread A and B can pass here simultaneously
scheduledExecutorService = Executors.newSingleThreadScheduledExecutor(); // Executed twice
// ...
}
### What you expected to happen
The `scheduledExecutorService` should be initialized as a Singleton in a thread-safe manner (e.g., using Double-Checked Locking). Only one thread pool should be created to avoid thread leaks.
### Anything else
### Proposed Solution (Double-Checked Locking)
I suggest modifying the code to use synchronized DCL:
private void scheduleServiceNamesLookup(final URL url, final NacosAggregateListener listener) {
if (scheduledExecutorService == null) {
synchronized (this) {
if (scheduledExecutorService == null) {
// Recommend naming the thread for better debuggability
scheduledExecutorService = Executors.newSingleThreadScheduledExecutor(
new NamedThreadFactory("Dubbo-Nacos-Registry-Scheduler", true)
);
scheduledExecutorService.scheduleAtFixedRate(
() -> {
// ... existing logic ...
},
LOOKUP_INTERVAL,
LOOKUP_INTERVAL,
TimeUnit.SECONDS);
}
}
}
}
### Are you willing to submit a pull request to fix on your own?
- [x] Yes I am willing to submit a pull request on my own!
### Code of Conduct
- [x] I agree to follow this project's [Code of Conduct](https://www.apache.org/foundation/policies/conduct)
Contributor guide
Research direction
Locate org.apache.dubbo.registry.nacos.NacosRegistry and read scheduleServiceNamesLookup, focusing on the lazy scheduledExecutorService initialization and its concurrent callers. Validate the race with the project’s relevant Nacos registry tests or a focused concurrent reproduction; done means concurrent calls create only one scheduler without a thread leak.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java
- Domain
- backend, distributed-systems
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Stale
- Clarity
- Clearly specified
- Newbie friendliness
- 35/100