ReentrantReadWriteLock gets stuck on unlock

Question

I have a class that is used to acquire and release locks for files. I use a customKey class that is just a ReentrantReadWriteLock with an id string (id being the file). For some reason, this only works in some situations, and in most it hangs on unlock of all things - my debugger traces it's use all the way there and then just gets stuck.

What am I doing wrong? I would get if a thread crashed and did not release it's lock but here a thread tries to call unlock and does not go any further.

This is the method for locking:

override fun acquire(lockId: String?, ownerId: String?, sequence: Long): Boolean
{
    if (lockId != null)
    {
        lockedList.find { customLock -> customLock.Id == lockId }.apply {
            if (this != null) //lock already exists for this ID
            {
                println("Locking file $lockId Existing lock")
                this.writeLock().lock()
                println("Locked file $lockId")
            } else //lock does not exist
            {
                val newLock = CustomLock(lockId)
                lockedList.add(newLock)
                println("Locking file $lockId")
                newLock.writeLock().lock()
                println("Locked file $lockId")
            }
        }
        return true
    } else
    {
        throw InvalidParameterException("ERROR: lockId or ownerId is null!")
    }
}

This is the method for releasing:

override fun release(lockId: String?, ownerId: String?)
    {
        if (lockId != null)
        {
            lockedList.find { customLock -> customLock.Id == lockId }.apply {
                if (this != null)
                {
                    println("Unlocking file $lockId")
                    this.writeLock().unlock()
                    if (this.isWriteLocked)
                    {
                        throw Exception("ERROR: Unlocking failed!")
                    }
                } else
                {
                    throw Exception("ERROR: Lock not found!")
                }
            }
        }
    }

Please do not bother to talk about architecture, this one is dictated by the assignment. Also please ignore the ownerId and sequence variables.

EDIT: I tried using just a single lock and while not very efficient, it did work, so @gidds may be onto something, but neither ConcurrentHashMap nor ConcurrentLinkedQueue (it is simpler to replace a List with) solved the problem.

EDIT2: This is the new class where I used the ConcurrentHashMap. It still does not work correctly, can anyone please point out where I messed up? Thanks

class LockServer(port: Int) : LockConnector, RemoteException()
{
private val lockedList = ConcurrentHashMap<String, CustomLock>()
private var registry: Registry = LocateRegistry.createRegistry(port)

init
{
    registry.bind(ServiceNames.LockService.toString(), UnicastRemoteObject.exportObject(this, port))
}

/**
 * Method acquire() should block the multiple calls from the clients for each specific lockId string.
 * It means when one client acquires the lock "A" and the "A" is not locked by any other clients,
 * the method should record the lock and return true. If the "A" is already locked by any other client,
 * the method is blocked and continues only after the lock "A" is released.
 * (Note: Return value false is not used in this basic implementation.
 * Parameters ownerId and sequence are also not used in this basic implementation.)
 */
override fun acquire(lockId: String?, ownerId: String?, sequence: Long): Boolean
{
    if (lockId != null)
    {
        lockedList.computeIfPresent(lockId){id, value ->
            println("Locking file $id Existing lock")
            value.writeLock().lock()
            println("Locked file $id")
            return@computeIfPresent value
        }
        lockedList.computeIfAbsent(lockId){
            val newLock = CustomLock(it)
            println("Locking file $lockId")
            newLock.writeLock().lock()
            println("Locked file $lockId")
            return@computeIfAbsent newLock
        }
        return true
    } else
    {
        throw InvalidParameterException("ERROR: lockId or ownerId is null!")
    }
}

/**
 * Method release() should release the lock and unblock all waiting acquire() calls for the same lock.
 * (Note: Parameter ownerId is not used in this basic implementation.)
 */
override fun release(lockId: String?, ownerId: String?)
{
    if (lockId != null)
    {
        lockedList.computeIfPresent(lockId){ id, value ->
            println("Unlocking file $id")
            value.writeLock().unlock()
            println("Unlocked file $id")
            return@computeIfPresent value
        }
    }
}

/**
 * Method stop() unbinds the current server object from the RMI registry and unexports it.
 */
override fun stop()
{
    registry.unbind(ServiceNames.LockService.toString())
}

}

EDIT3: New implementation for acquire:

lockedList.compute(lockId){id, value ->
            if (value == null)
            {
                println("Locking file $id")
                val newLock = CustomLock(id)
                newLock.writeLock().lock()
                println("Locked file $id")
                return@compute newLock
            }
            println("Locking file $id Existing lock")
            value.writeLock().lock()
            println("Locked file $id")
            return@compute value
        }

and for release:

println("Unlocking $lockId")
        lockedList[lockId]!!.writeLock().unlock()
        println("Unlocked $lockId")

Still the same failure

Answer 1

This may not be the problem of LockServer class, but the ones that are using it:

thread1:

acquire("file1")
acquire("file2")
release("file2")
release("file1")

thread2:

acquire("file2")
acquire("file1")
release("file1")
release("file2")

And it happened that the execution order was the following:

thread1.acquire("file1")
thread2.acquire("file2")
thread1.acquire("file2") //locked by thread2, waiting
thread2.acquire("file1") //locked by thread1... BOOM, deadlock!

UPD.:

Consider using tryLock() (possibly with some timeout) instead of simple lock() for existing locks:

    fun tryAcquire(lockId: String?, timeout: Long, unit: TimeUnit): Boolean {
        if (lockId != null) {
            var success = false
            lockedList.compute(lockId) { id, value ->
                if (value == null) {
                    println("Locking file $id")
                    val newLock = CustomLock(id)
                    newLock.writeLock().lock()
                    success = true
                    println("Locked file $id")
                    return@compute newLock
                }
                println("Locking file $id Existing lock")
                val lock = value.writeLock()
                if (lock.tryLock() || lock.tryLock(timeout, unit)) {
                    success = true
                    println("Locked file $id")
                }
                return@compute value
            }
            return success
        } else {
            throw InvalidParameterException("ERROR: lockId or ownerId is null!")
        }
    }

Answer 2

This may not be your problem, but the code has a race condition when adding a new lock: if two threads try to lock the same (new) file, they could both create a lock for it.  Both locks would get added to the list, but only the first would be found after that.  (This assumes that the list itself is thread-safe; otherwise one of the adds could fail, loop forever, or leave the list in an inconsistent state and crash later.)

You could fix this with some synchronisation.  But a better approach might be to store the locks in a ConcurrentHashMap (keyed on the lock ID) instead of a list, and use an atomic operation such as computeIfAbsent() to create a new lock safely.  (That would also improve asymptotic performance, as it would avoid scanning a list each time.)

Also, as a matter of style, the use of apply() on the lock looks a bit awkward.  (Its usual use is for customising a newly-created object.)  I think let() would be more idiomatic there; you'd just have to change this to it inside.  Or use an old-fashioned temporary variable, of course.