This is an automated email from the ASF dual-hosted git repository.
nicholasjiang pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/celeborn.git
The following commit(s) were added to refs/heads/main by this push:
new c1fb94d6e [CELEBORN-1910] Remove redundant synchronized of
isTerminated in ThreadUtils#sameThreadExecutorService
c1fb94d6e is described below
commit c1fb94d6e3b13271c69fe54c03f75361658b4597
Author: SteNicholas <[email protected]>
AuthorDate: Thu Mar 13 16:10:07 2025 +0800
[CELEBORN-1910] Remove redundant synchronized of isTerminated in
ThreadUtils#sameThreadExecutorService
### What changes were proposed in this pull request?
Remove the redundant synchronized keyword from the `isTerminated` method in
`sameThreadExecutorService()` implementation within the `ThreadUtils` class.
The method was using both a synchronized block and an explicit `ReentrantLock`,
which is unnecessary and potentially problematic.
Backport https://github.com/apache/spark/pull/50210.
### Why are the changes needed?
The changes are needed for several reasons:
1. **Eliminates redundant synchronization**: The current implementation
uses both synchronized keyword and explicit ReentrantLock, which is redundant
and creates unnecessary overhead.
2. **Reduces potential deadlock risks**: Using two different locking
mechanisms (built-in synchronized and explicit `ReentrantLock`) in the same
method could lead to complex lock ordering issues and increase deadlock risks.
Although the risk of deadlock in the current implementation is low, if someone
modifies the code in the future and adds a method that acquires these two locks
in a different order, it would introduce a deadlock risk.
3. **Improves performance**: Removing the unnecessary synchronization layer
reduces lock contention and context switching overhead.
4. **Code consistency**: Other methods in the same class only use
`ReentrantLock` for synchronization, so removing synchronized makes the code
style more consistent.
5. **More precise control**: `ReentrantLock` already provides all the
synchronization needed with more features than the implicit synchronized
keyword.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
CI.
Closes #3150 from SteNicholas/CELEBORN-1910.
Authored-by: SteNicholas <[email protected]>
Signed-off-by: SteNicholas <[email protected]>
---
common/src/main/scala/org/apache/celeborn/common/util/ThreadUtils.scala | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git
a/common/src/main/scala/org/apache/celeborn/common/util/ThreadUtils.scala
b/common/src/main/scala/org/apache/celeborn/common/util/ThreadUtils.scala
index 33c6d7c82..d745e8b08 100644
--- a/common/src/main/scala/org/apache/celeborn/common/util/ThreadUtils.scala
+++ b/common/src/main/scala/org/apache/celeborn/common/util/ThreadUtils.scala
@@ -67,7 +67,7 @@ object ThreadUtils {
}
}
- override def isTerminated: Boolean = synchronized {
+ override def isTerminated: Boolean = {
lock.lock()
try {
serviceIsShutdown && runningTasks == 0
