-
Notifications
You must be signed in to change notification settings - Fork 26.6k
[Fix-15886] Fix thread leak in NacosRegistry using Double-Checked Locking #15915
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: 3.3
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 3.3 #15915 +/- ##
=========================================
Coverage 60.71% 60.72%
- Complexity 11769 11775 +6
=========================================
Files 1948 1948
Lines 88732 88735 +3
Branches 13379 13380 +1
=========================================
+ Hits 53877 53886 +9
+ Misses 29325 29321 -4
+ Partials 5530 5528 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Regarding the Codecov report: Since this PR fixes a race condition via Double-Checked Locking, it is difficult to reproduce the concurrency scenario deterministically in unit tests, hence the 0% coverage on the new lines |
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 PR fixes a thread leak issue in NacosRegistry caused by a race condition during lazy initialization of the scheduledExecutorService. The fix implements double-checked locking to ensure the executor service is created only once, even when multiple threads attempt concurrent initialization.
Key Changes:
- Applied double-checked locking pattern in
scheduleServiceNamesLookupmethod to prevent multiple executor service instances from being created - Added synchronized block around executor initialization to ensure thread-safe singleton instantiation
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| TimeUnit.SECONDS); | ||
| synchronized (this) { | ||
| if (scheduledExecutorService == null) { | ||
| scheduledExecutorService = Executors.newSingleThreadScheduledExecutor(); |
Copilot
AI
Dec 24, 2025
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.
Potential race condition. This assignment to scheduledExecutorService is visible to other threads before the subsequent statements are executed.
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.
Verified that scheduledExecutorService is declared as volatile, so this DCL pattern is thread-safe.
What is the purpose of the change
Fixes #15886
This PR fixes a thread leak issue in
NacosRegistry.java.The method
scheduleServiceNamesLookuphad a race condition where multiple threads could pass theif (scheduledExecutorService == null)check simultaneously, leading to the creation of multipleScheduledExecutorServiceinstances that were never shut down.Brief changelog
scheduleServiceNamesLookupto ensure the executor service is instantiated safely and only once.Verifying this change