0

Given a system where users can access different services through the API for conducting business transactions, I am currently working on integrating a read-write lock into the system. One approach I am considering is manually adding the locking logic to each method within the service:

class SomeService {
    func DoBusinessLogic(res1 Resource, res2 Resource, res3 Resource) {
        rwlock.r_lock(res)
        rwlock.w_lock(res2)
        rwlock.w_lock(res3)

        // perform actual business logic

        rwlock.r_unlock(res)
        rwlock.w_unlock(res2)
        rwlock.w_unlock(res3)
    }

    ...
}

However, since the system has numerous services and each service has multiple methods, this approach violates the Don't Repeat Yourself (DRY) principle. It would require going through each method individually to lock the resources accordingly.

Creating a common module that dynamically inspects the parameters and locks the resources seemed impractical, as different methods could require different and varying numbers of resources.

I am seeking guidance on the best practice in this situation.

4 Answers 4

3

Not quite highlighted by other commentators, but:

    rwlock.r_lock(res)
    rwlock.w_lock(res2)
    rwlock.w_lock(res3)

This pattern is extremely dangerous: if another piece of code ever does

    rwlock.r_lock(res)
    rwlock.w_lock(res3)
    rwlock.w_lock(res2)

then there is a risk that one will end up holding res3 and the other will hold res2: a deadlock. Holding more than one lock at once is a "code smell".

If you can refactor your structure to have a single lock held that covers all relevant resources, use lockless data structures, or more transaction-structured systems, that will help avoid deadlock.

2
  • Here’s a systematic method: Every lock is assigned a level from 0 upwards. While you hold a lock at level n you must only get locks at lower levels. This avoids any potential deadlock. So one of your two sequences would have been illegal, depending on how these levels were assigned.
    – gnasher729
    Dec 22, 2023 at 14:13
  • Yeah. A common implementation of that is: (1.) Every resource has a single well-known name, (2.) A client that acquires multiple resources always does so in lexical order of resource names, and (3.) Client shall release resources in reverse order. Thus the collation order amounts to a description of lock levels.
    – J_H
    Dec 22, 2023 at 18:12
3

The DRY principle is about

  • not repeating identical logic (and logic which must be identical) by centralizing such logic in one place.

It is not about

  • "similar looking code" in general, or code which performs the same things "by chance".

In your case, as you noted by yourself, each service may require a lock on different resources, and also the distinction between read or write lock is an individual one. So each service might need a different locking logic, and even if two services currently need the same locking logic "by chance", it is not a good idea to couple their locking logic together.

The only DRY violation I see in your code is that the locking sequence in each method must match the unlocking sequence - the logic which resources to lock and how is always in two places. That is error prone, when one needs to change the locking sequence at the beginning, one has to make sure not to forget to change the sequence at the end in the corresponding manner.

The solution to this, however, isn't necessarily language agnostic. In C++, one can make use of RAII and let the unlock happen automatically in the destructor of some lock object. In C#, one would make use of the Dispose pattern and create locks with the "using" syntax. Java has Automatic Resource Block Management since version 7. Other programming languages may provide different means of scope-based resource management.

A mostly language agnostic strategy to solve this would be this one:

  • create methods which can lock and unlock a sequence of resources by some kind of description (maybe simply two resource lists, one for read locks, another one for write locks)

  • define that description once in the code at the beginning of the scope, assign it to some variable and pass it at the beginning and the end of each function to the new methods.

So pick what matches your environment and requirements most.

2
  • well said. I suppose in either approach, we need to leave some code in the method, either by locking the resource directly, or passing some description to a common module to lock the resources. I wonder if completely removing this part from the business logic method is possible since locking the resource is not technically part of the actual business logic.
    – YiLuo
    Dec 26, 2023 at 4:18
  • @YiLuo:maybe thats possible with aspect oriented programming, see kentlai.com/writings/locks-via-spring-aop.html for example. If thats practible is a different question.
    – Doc Brown
    Dec 26, 2023 at 5:09
2

A few potential improvements to consider:

  • You can split the unlocking/locking parts into separate function(s).
  • Those functions might be reusable, depending on your system.
  • You could look at passing in an IEnumerable of resources to lock / unlock, correctly ordered.

DRY is a useful reminder about how to correctly structure code, but don't die on that hill either. A friend of mine prefers DRYTM or Don't Repeat Yourself Too Much - there are times when trying to eliminate a little casual repetition can introduce a lot more complexity than it's worth.

2

I think you could pass the locking object into the resource calls

So something like:

Resource : ILockable {
  DoThing(Scope lock)
  {
     if(lock != null) {
        lock.subscribe(this, doThing) //acquires lock
     }
  }
  doThing() {
    //actual operation
  }
}

Scope {
  subscribe(ILockable obj)
  {
     this.lockedObjects += obj, operation
     lock(obj)
  }
  dispose()
  {
    foreach(var obj in lockedObjects)
    {
      //run operation
    }
    foreach(var obj in lockedObjects)
    {
      unlock(obj)
    }
  }
}


Worker
  DoThings()
  {
    var scope = new Scope();
    res1.DoThing(scope);
    res2.DoThing2(scope);
    res3.DoThing3(scope);
    scope.Dispose()
  }

Now the resource can be in charge of how it locks for each operation, the locking code is separated into the scope object and interfaces and the unlocking is guaranteed when the scope object falls out of scope.

1
  • It seems to me that we still need a way to lock ALL resources before DoThing*, since between the DoThing* calls the resources could've been modified by others, and it'd cause unexpected errors if manipulating one resource require one another.
    – YiLuo
    Dec 26, 2023 at 4:07

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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