2
\$\begingroup\$

Surprisingly, this question has no satisfactory answer. A safe usage of a LockCloseable would be like

try (LockCloseable lockCloseable = LockCloseable.lock(lock)) {
    doSomethingUnderLock();
} // automatic release

or using @lombok.Cleanup

{
    @Cleanup LockCloseable lockCloseable = LockCloseable.lock(lock)
    doSomethingUnderLock();
} // automatic release when the scope is left

while all answers either create a new lock (non-sense) or when locking return an existing object (dangerous) or return void (unusable with try-with resources).

So I'm giving it a try:

@RequiredArgsConstructor(access=AccessLevel.PRIVATE)
public final class MgLockCloseable implements AutoCloseable {
    public static MgLockCloseable tryLock(Lock lock) {
        return new MgLockCloseable(lock.tryLock() ? lock : null);
    }

    public static MgLockCloseable lock(Lock lock) {
        lock.lock();
        return new MgLockCloseable(lock);
    }

    @Override public void close() {
        if (isLocked()) {
            lock.unlock();
        }
    }

    public boolean isLocked() {
        return lock != null;
    }

    @Nullable private final Lock lock;
}

Notes:

  • RequiredArgsConstructor comes from lombok and does exactly what the name says.
  • I implemented only the two methods I need.
  • It's only AutoCloseable as the closing is not idempotent.
\$\endgroup\$
4
\$\begingroup\$

MgLockCloseable.tryLock() is not practical if acquired in a loop. This is how one would do this traditionally:

while (!lock.tryLock()) {
    sleep();
}
try {
    // do locked stuff
} finally {
    lock.unlock();
}

This is what is necessary to do this with MgLockClosable:

boolean processed = false;
while (!processed) {
    try (AutoClosable closable = MgLockClosable.tryLock()) {
        if (closable.isLocked()) {
            processed = true;
            // do locked stuff
        } else {
            sleep();
        }
    }
}

From the javadoc of AutoClosable.close():

implementers of this interface are strongly encouraged to make their close methods idempotent.

It's easy to make the close() implementation idempotent and is useful in some scenarios:

try (MgLockClosable writeLocked = MgLockClosable.lock(getLock().writeLock()) {
    try (MgLockClosable ignored = MgLockClosable.lock(getLock().readLock()) {
        // do write lock related stuff
        writeLocked.close();
        // do write lock unrelated stuff (potentially slow)
    }
}

The code demonstrates read/write lock downgrading. Splitting the logic into two blocks, the one requiring a write lock, and the one requiring a read lock, has the benefit of improved concurrency. At the same time the code is still save, since both locks will be released in any case.


I'd swap the two lines in lock(). If the constructor throws (for any reason we don't know at the moment) the lock will never be unlocked. On the other hand, if you swap the lines: if the constructor throws, the lock will not be acquired. If Lock.lock() throws the instance of MgLockCloseable will never be returned.

\$\endgroup\$
  • \$\begingroup\$ Is there a need to call tryLock() in a loop when there's tryLock(long time, TimeUnit timeUnit)? Even when using a loop, you could simplify it with break. \$\endgroup\$ – maaartinus Mar 20 '17 at 13:02
  • \$\begingroup\$ True. But MgLockClosable does not expose tryLock(long, TimeUnit). break would simplify the code but not significantly. \$\endgroup\$ – Eduard Wirch Mar 20 '17 at 19:53
  • \$\begingroup\$ True. It surely should expose tryLock(long, TimeUnit) and this can be trivially done, but I only implemented what I needed. +1 for idempotency and line swapping. \$\endgroup\$ – maaartinus Mar 21 '17 at 6:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.