Copilot commented on code in PR #3643:
URL: https://github.com/apache/celeborn/pull/3643#discussion_r3021686199
##########
worker/src/main/java/org/apache/celeborn/service/deploy/worker/memory/MemoryManager.java:
##########
@@ -109,7 +109,11 @@ public static MemoryManager initialize(CelebornConf conf) {
public static MemoryManager initialize(
CelebornConf conf, StorageManager storageManager, AbstractSource source)
{
if (_INSTANCE == null) {
- _INSTANCE = new MemoryManager(conf, storageManager, source);
+ synchronized (MemoryManager.class) {
+ if (_INSTANCE == null) {
+ _INSTANCE = new MemoryManager(conf, storageManager, source);
+ }
+ }
}
return _INSTANCE;
Review Comment:
Double-checked locking is only correct in Java if `_INSTANCE` is declared
`volatile` (otherwise another thread can observe a partially constructed
`MemoryManager` via write reordering, since the first null-check happens
outside the synchronized block). Please ensure the `_INSTANCE` field is
`private static volatile MemoryManager _INSTANCE;` or alternatively remove the
outer null-check and always synchronize reads/writes.
##########
worker/src/main/java/org/apache/celeborn/service/deploy/worker/memory/MemoryManager.java:
##########
@@ -598,7 +602,7 @@ public ByteBufAllocator getStorageByteBufAllocator() {
}
@VisibleForTesting
- public static void reset() {
+ public static synchronized void reset() {
_INSTANCE = null;
}
Review Comment:
The PR description marks 'Does this PR resolve a correctness bug? No', but
the linked stack trace shows an actual `NullPointerException` likely caused by
a race during initialization. Consider updating the PR metadata/description to
reflect that this addresses a correctness/concurrency bug (even if user-facing
behavior is unchanged).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
