Skip to content

Commit

Permalink
[MINOR] refactor: Calling lock() method outside try block to avoid un…
Browse files Browse the repository at this point in the history
…necessary errors (#1590)

### What changes were proposed in this pull request?

Calling `lock()` method outside try block to avoid unnecessary errors

### Why are the changes needed?

In general, the `lock()` method should be placed outside the try block. The reason is that if the `lock()` method throws an exception, it indicates that the lock was not acquired, so there is no need to attempt to release it in the finally block. If the `lock()` method is placed within the try block and it throws an exception, the finally block will still be executed and attempt to release a lock that was never acquired, leading to errors.

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Existing UTs.
  • Loading branch information
rickyma committed Mar 21, 2024
1 parent dd67774 commit 32d533d
Show file tree
Hide file tree
Showing 4 changed files with 6 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -297,8 +297,8 @@ public void run() {
} catch (Throwable t) {
LOG.warn("send shuffle data exception ", t);
} finally {
memoryLock.lock();
try {
memoryLock.lock();
if (LOG.isDebugEnabled()) {
LOG.debug("memoryUsedSize {} decrease {}", memoryUsedSize, size);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -433,8 +433,8 @@ private class ReporterCallable extends CallableWithNdc<Void> {
protected Void callInternal() throws Exception {
long nextReport = 0;
while (!isShutdown.get()) {
reportLock.lock();
try {
reportLock.lock();
while (failedEvents.isEmpty()) {
boolean signaled =
reportCondition.await(maxTimeToWaitForReportMillis, TimeUnit.MILLISECONDS);
Expand Down Expand Up @@ -1175,8 +1175,8 @@ public void fetchFailed(
srcAttemptIdentifier.getInputIdentifier(),
srcAttemptIdentifier.getAttemptNumber());
if (maxTimeToWaitForReportMillis > 0) {
reportLock.lock();
try {
reportLock.lock();
failedEvents.merge(readError, 1, (a, b) -> a + b);
reportCondition.signal();
} finally {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -281,8 +281,8 @@ public void run() {
} catch (Throwable t) {
LOG.warn("send shuffle data exception ", t);
} finally {
memoryLock.lock();
try {
memoryLock.lock();
if (LOG.isDebugEnabled()) {
LOG.debug("memoryUsedSize {} decrease {}", memoryUsedSize, size);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -267,8 +267,8 @@ public StatusCode registerShuffle(
ShuffleDataDistributionType dataDistType,
int maxConcurrencyPerPartitionToWrite) {
ReentrantReadWriteLock.WriteLock lock = getAppWriteLock(appId);
lock.lock();
try {
lock.lock();
refreshAppId(appId);

ShuffleTaskInfo taskInfo = shuffleTaskInfos.get(appId);
Expand Down Expand Up @@ -753,8 +753,8 @@ public void checkLeakShuffleData() {
@VisibleForTesting
public void removeResources(String appId, boolean checkAppExpired) {
Lock lock = getAppWriteLock(appId);
lock.lock();
try {
lock.lock();
LOG.info("Start remove resource for appId[" + appId + "]");
if (checkAppExpired && !isAppExpired(appId)) {
LOG.info(
Expand Down

0 comments on commit 32d533d

Please sign in to comment.