1

In a class I'm maintaining I have 2 methods using a common lock on part of their codes:

class A
{
    private readonly object o = new object();
    public void F1(B b)
    {
        ...
        lock (o)
        {
            ...
            b.Data.Add(...);
            ...
        }
        ...
    }
    public void F2(B b)
    {
        ...
        lock (o)
        {
            ...
            b.Data.Add(...);
            ...
        }
        ...
    }
}

So the locked sections cannot be run concurrently even if working on different B entities.
They do not write any external state (nor read any by the way) and only change the state of their parameters.

In order to protect access to parameters I would do this instead:

lock(b)
{
    ...
}

Note that each Data is unique to each B instance and only edited there.
And other properties of b can be edited inside the locked section so b seems the right level to lock to group all these changes to guarantee atomicity.

  1. Can I safely remove the global lock on o and instead use the more granular one on b?
  2. If no, what could be the pitfalls?
Pragmateek
  • 13,174
  • 9
  • 74
  • 108
  • 2
    Most likely this locking is being done at the wrong level. Since B is passed in as a parameter, ideally, you would either put the locking INSIDE of the B instance so that multiple threads cannot separately mutate the state of that instance, or properly ensure that _ONLY_ `A` is able to mutate any instance of `B`. However, as currently written, you can only update one B at a time in `A` since the lock will prevent any other threads from acting on separate `B` instances. – David L Feb 24 '23 at 20:02
  • 1
    It's impossible to say whether it's safe based on the information you've given, and there are plenty of scenarios that would be unsafe with the suggested change. An obvious one being if two distinct instances of `B`s hold the same `Data`, locking on the instance of `B` passed to these functions would not prevent concurrent writes to the shared `Data`. Even if this isn't the case, by locking on something "external" (like the instance of `B` passed to your method) there's the possibility that some other code also locks on that instance of `B`, introducing contention or even deadlocks. – Iridium Feb 24 '23 at 20:03
  • Whoever going to vote to re-open please make sure to address what is currently looking like a duplicate - https://stackoverflow.com/questions/251391/why-is-lockthis-bad before voting. Pragmateek - consider to make that [edit] yourself when you provide other details requested in previous comments. – Alexei Levenkov Feb 24 '23 at 21:40
  • @DavidL Indeed putting the responsibility of thread safety on each B instance seems better than rewriting it for each usage. @Iridium You're right for access to Data, but the issue was already there by locking on the global `o` lock, and locking on `b` should not increase the risks (and in this case each `Data` is unique for each `B` instance). As for deadlock not sure how it could happen as there is no other lock so no risk of "crossing" (locking them in a different order). – Pragmateek Feb 25 '23 at 15:07
  • @AlexeiLevenkov No problem :) please tell me which details you'd need. – Pragmateek Feb 25 '23 at 15:10
  • 1
    @TheodorZoulias Indeed, often closing means death, as many are willing to close but only a few do take the time to understand the question further and ask for reopening :( – Pragmateek Feb 25 '23 at 15:10
  • @AlexeiLevenkov I've made some edits to hopefully clarify the question, let me know if you need others :) – Pragmateek Feb 25 '23 at 15:18
  • 2
    @Pragmateek Thanks for the edits. I think the question is unique enough to be potentially answered now. – David L Feb 25 '23 at 19:24

1 Answers1

2

Can I safely remove the global lock on o and instead use the more granular one on b?

Yes, doing lock (b) is safe, under these conditions:

  1. The B class is completely under your control. You are not exposing it either directly or through an interface to external code.
  2. The operations on the B instance that are protected by the lock (b) in the F1 and F2 methods, are also protected using the same locker everywhere else. Code internal to B should do lock (this), and code external to B should do lock (b).
  3. The operations on the B instance that are protected by the lock (b), do not include operations on other B instances. Otherwise it is possible for a deadlock to occur, in case for example that the b1 is waiting on the lock (b2) and the b2 is waiting on the lock (b1). For more details see the dining philosophers problem.
Theodor Zoulias
  • 34,835
  • 7
  • 69
  • 104
  • 1
    great answer; thanks for including 3 - that's the one that usually catches people out – Marc Gravell Feb 27 '23 at 11:17
  • 1
    Thanks for your answer. :) 1. Yes 2. No but it won't be worth by locking on `b` instead of `o` ;-) 3. Yes, `B` instances are independent from each others – Pragmateek Feb 27 '23 at 11:43