Back to all reviewers

Always secure your locks

deeplearning4j/deeplearning4j
Based on 3 comments
Java

When using locks or other synchronization mechanisms in concurrent code, always release locks in a finally block to ensure they are released even when exceptions occur. Failure to do so can lead to deadlocks if other threads need the same lock.

Concurrency Java

Reviewer Prompt

When using locks or other synchronization mechanisms in concurrent code, always release locks in a finally block to ensure they are released even when exceptions occur. Failure to do so can lead to deadlocks if other threads need the same lock.

Additionally, choose the appropriate concurrency primitive for your specific use case. Using the wrong tool (like a Semaphore when a CountDownLatch is more appropriate) can lead to subtle bugs that are difficult to diagnose.

Bad example:

public void updateModel(@NonNull Model model) {
    modelLock.writeLock().lock();
    // If an exception occurs here, lock is never released!
    // Do work with the model...
    modelLock.writeLock().unlock();
}

Good example:

public void updateModel(@NonNull Model model) {
    modelLock.writeLock().lock();
    try {
        // Do work with the model...
    } finally {
        modelLock.writeLock().unlock();
    }
}

For wait/notify patterns, consider whether a CountDownLatch or other higher-level concurrency primitive would be more appropriate than using raw locks or semaphores:

// Instead of this:
private Semaphore semaphore = new Semaphore(0);
public void update(Observable o, Object arg) {
    semaphore.release(Integer.MAX_VALUE); // Potential overflow!
}

// Consider this:
private CountDownLatch latch = new CountDownLatch(1);
public void update(Observable o, Object arg) {
    latch.countDown(); // Safe, can only transition once
}

public void waitTillDone() throws InterruptedException {
    latch.await(); // Properly propagate interruption
}
3
Comments Analyzed
Java
Primary Language
Concurrency
Category

Source Discussions