0

I found the below code in our application. As per my understanding of how lock works, I cannot see how this provides any benefit, or that removing this can cause any issues.

    private static object _lockTransferInProgress = new object();
    private static volatile bool _transferInProgress = false;
    public static bool Dialer_TransferInProgress
    {
        get { return _transferInProgress; }
        set
        {
            lock (_lockTransferInProgress)
            {
                _transferInProgress = value;
            }
        }
    }
  1. Does this provide any benefits?
  2. Are there risks to removing this?
Juan Venter
  • 133
  • 1
  • 10
  • You also need to bear in mind that the variable is marked [`volatile`](https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/volatile) – Liam Aug 30 '18 at 12:35
  • The lock prevents another thread from entering that code block while another thread is inside it, yes there is a risk in removing it. – Ryan Wilson Aug 30 '18 at 12:35
  • This question is hard to answer without context. There could be very good reason why the person has flagged this volatile and added locking or these reason could be entirely spurious. – Liam Aug 30 '18 at 12:36
  • 2
    According to the duplicate's answer you need a lock **or** volatile, not both. The `lock` is more expensive so i'd remove it. – Tim Schmelter Aug 30 '18 at 12:37
  • Assignment of a bool variable in .NET is atomic so no need for a lock. – Dmytro Mukalov Aug 30 '18 at 12:39
  • Though each has a slightly different semantic. – Liam Aug 30 '18 at 12:39
  • @DmytroMukalov that's [not what Mark says](https://stackoverflow.com/a/1222325/542251) – Liam Aug 30 '18 at 12:40
  • 1
    @TimSchmelter: I don't agree on the the duplicate mark. The duplicate's answer is about **accessing**, this question is about **assignment**. Even if this is probably also discussed somewhere. – ChristianMurschall Aug 30 '18 at 12:44
  • @Liam, That's what Eric [says](https://blogs.msdn.microsoft.com/ericlippert/2011/05/31/atomicity-volatility-and-immutability-are-different-part-two/). And also should add that you misinterpreted the Marc's answer as he discussed access which non atomic verification `if(_cancelled)` which is absolutely different story. – Dmytro Mukalov Aug 30 '18 at 12:48
  • @linuxrocks: the other question is also about assignment which is done in a click-event of a button, so the GUI thread. He also mentioned that he needed to lock this `bool` in the event handler too. That's why he asks if the lock is necessary at all. So for me that is a duplicate, it's even about the same type(`bool`). Note that you need a `lock` even when you just _access_ something that is protected by a `lock` somewhere else. – Tim Schmelter Aug 30 '18 at 12:49
  • 1
    @DmytroMukalov: i don't understand in what way Eric L. says something else than Marc G. Yes, it's an atomic operation to assign a value to a `bool` field. But that doesn't prevent you from compiler "optimizations" that threads cache the value in their registers, so one thread never sees the updated value. `Volatile` does this as [Marc](https://stackoverflow.com/a/1222325/284240) has mentioned. – Tim Schmelter Aug 30 '18 at 12:57
  • @TimSchmelter If you check my comments carefully there is no mention of fact that volatile is needed or not needed here, I stated that lock isn't need here because it merely guarantees atomically of assignment which is already atomic. – Dmytro Mukalov Aug 30 '18 at 13:00
  • @TimSchmelter As for additional considerations regarding volatile it's just not enough information in the given piece of code to make any further conclusions as it's not clear how this variable is used. – Dmytro Mukalov Aug 30 '18 at 13:06
  • 1
    @DmytroMukalov: it also provides a memory barrier to ensure that all threads get the most recent value. So you either have to use a lock or volatile. By the way, meanwhile i have read that E. Lippert discourages from ever using volatile fields. He would use a lock. https://blogs.msdn.microsoft.com/ericlippert/2011/06/16/atomicity-volatility-and-immutability-are-different-part-three/ (_"I discourage you from ever making a volatile field"_) – Tim Schmelter Aug 30 '18 at 13:08
  • @DmytroMukalov: sure, maybe he can remove the lock and volatile without any problems. But since this is old code there might be a reason and i wouldnt say it's safe to remove – Tim Schmelter Aug 30 '18 at 13:10
  • @TimSchmelter I know what it does, but again we don't know if a code which verifies the variable can be wrapped with lock so only thing can be clearly stated here that lock ins't needed at that place but volatile may be needed may be not needed. – Dmytro Mukalov Aug 30 '18 at 13:11
  • @DmytroMukalov: _"but volatile may be needed may be not needed"_ If you've read my comment about E. Lipperts opinion you wouldn't have made this conclusion – Tim Schmelter Aug 30 '18 at 13:18
  • Let us [continue this discussion in chat](https://chat.stackoverflow.com/rooms/179110/discussion-between-dmytro-mukalov-and-tim-schmelter). – Dmytro Mukalov Aug 30 '18 at 13:20

0 Answers0