4 ms·

What makes SRWLock buggy or over complicated?

Wumpnot 2y ago
In some rare cases you can get an exclusive lock when you asked for a shared lock, most code won't care and will still work correctly, but sometimes people do weird shit that they probably should have used a different construct for, and run into this.
- colonwqbang 2y ago
  The point of an RW lock is that it supports multiple concurrent readers. Why is it weird to expect a non exclusive lock operation to be non exclusive?
  - tialaramex 2y ago
    It's fair to say that most people can't hit this bug, or rather, they can't hit the grave consequence (a deadlock). To deadlock you need to specifically need shared access when you asked for shared access. A lot of people have code where they want shared access for improved performance, but they don't need it. This bug makes their code a tiny bit slower in some edge cases (where it took the wrong lock), they're not overjoyed about the bug but it has no significant consequences.
- neerajsi 2y ago
  Thanks for the explanation. Sometimes it's worth it to keep weird shit working, but it's hard to predict which subset of weird shit will happen in practice.
tialaramex 2y ago
The idea of SRWLock is, as its name might suggest, it's a Simple Reader Writer Lock. This makes it in theory excellent for implementing this precise feature which you will see in for example C++ (the MSVC implementation is, in fact, just an SRWLock) or Rust So immediately it's over-complicated for a mutex because we don't need it to handle multiple readers, we're never taking its shareable reader lock, we (as you see in Raymond's example code) just rely entirely on the exclusive lock which is about 10% of its function. SRWLock is the size of a pointer - because of course in fact it is a pointer, to a lazily allocated structure - with some flag bits squirrelled away at the bottom of the pointer. The exciting bug is that at some point whoever was implementing this for Windows screwed up while implementing the (necessary for performance) unfair lock stealing strategy. Brief aside about "lock stealing": If I want to take a lock, and I notice that somebody else just released that lock, I can just take it and we're done - this is unfair because there may be a queue of waiters, and I cut in ahead of them, but it has better performance. This is a simple and common feature for a mutex - fair locks are sometimes useful but definitely should not be the default. Now, in SRWLock the lock stealing code doesn't remember whether you're trying to take the shared reader lock or the exclusive writer lock. So, it just always steals the exclusive lock - even when that's not what you wanted. As a result in code relying on SRWLock if you take the reader lock and then wait on somebody else to also get that lock (which they should be able to because it's a shared lock) this sometimes deadlocks. Oops.
- neerajsi 2y ago
  I've worked in and around srwlock for a long time. I don't think it's all that reasonable to think you should get "shared starve exclusive" by default, which is what you're asking for. We used to demand that kind of behavior in some windows filesystem code and it's always been a design mistake.
  - LegionMammal978 2y ago
    I don't think SharedStarveExclusive is what GP is talking about? From the description, it sounds like currently, a shared acquire immediately after an exclusive release will get silently 'upgraded' to an exclusive acquire in some cases (and remain exclusive until released), which can easily cause issues. And it can't even be detected or worked around, since the SRWLock functions don't return any information about the status of the lock. It looks like the Rust standard library migrated away from SRWLOCK on account of this issue [0]. [0] https://github.com/rust-lang/rust/issues/121949 https://github.com/rust-lang/rust/issues/121949
    - tialaramex 2y ago
      Yeah, as that Rust issue states, this was first replicated by a C++ programmer and STL (the person, not the Microsoft name for the C++ stdlib) said it looks like a bug in SRWLock. There's a Microsoft internal ticket which I can't read (but STL can because he's a Microsoft employee) but there's also a GitHub issue for STL (this time the stdlib, a GitHub project, although it was opened by the person) https://github.com/microsoft/STL/issues/4448 https://github.com/microsoft/STL/issues/4448 and it's confirmed in that issue this is a bug in SRWLock, thus no work for STL† (either the project or the person). It's not unusual (especially in C++ but this happens anywhere) that people would rather contort themselves to believe crazy things than accept that it's just a bug. † Both Windows and C++ holds themselves to a very low standard, "Yeah, it's broken, too bad" is acceptable. Once you've determined that it's a bug you're done.
      - neerajsi 2y ago
        Wow, thanks for the links. I was previously unaware of this issue. I checked the referenced bug and I see that it is fixed and will be released in an upcoming version of Windows.

In some rare cases you can get an exclusive lock when you asked for a shared lock, most code won't care and will still work correctly, but sometimes people do weird shit that they probably should have used a different construct for, and run into this.

colonwqbang 2y ago
The point of an RW lock is that it supports multiple concurrent readers. Why is it weird to expect a non exclusive lock operation to be non exclusive?
- tialaramex 2y ago
  It's fair to say that most people can't hit this bug, or rather, they can't hit the grave consequence (a deadlock). To deadlock you need to specifically need shared access when you asked for shared access. A lot of people have code where they want shared access for improved performance, but they don't need it. This bug makes their code a tiny bit slower in some edge cases (where it took the wrong lock), they're not overjoyed about the bug but it has no significant consequences.
neerajsi 2y ago
Thanks for the explanation. Sometimes it's worth it to keep weird shit working, but it's hard to predict which subset of weird shit will happen in practice.

tialaramex 2y ago

The idea of SRWLock is, as its name might suggest, it's a Simple Reader Writer Lock. This makes it in theory excellent for implementing this precise feature which you will see in for example C++ (the MSVC implementation is, in fact, just an SRWLock) or Rust So immediately it's over-complicated for a mutex because we don't need it to handle multiple readers, we're never taking its shareable reader lock, we (as you see in Raymond's example code) just rely entirely on the exclusive lock which is about 10% of its function. SRWLock is the size of a pointer - because of course in fact it is a pointer, to a lazily allocated structure - with some flag bits squirrelled away at the bottom of the pointer. The exciting bug is that at some point whoever was implementing this for Windows screwed up while implementing the (necessary for performance) unfair lock stealing strategy. Brief aside about "lock stealing": If I want to take a lock, and I notice that somebody else just released that lock, I can just take it and we're done - this is unfair because there may be a queue of waiters, and I cut in ahead of them, but it has better performance. This is a simple and common feature for a mutex - fair locks are sometimes useful but definitely should not be the default. Now, in SRWLock the lock stealing code doesn't remember whether you're trying to take the shared reader lock or the exclusive writer lock. So, it just always steals the exclusive lock - even when that's not what you wanted. As a result in code relying on SRWLock if you take the reader lock and then wait on somebody else to also get that lock (which they should be able to because it's a shared lock) this sometimes deadlocks. Oops.

neerajsi 2y ago
I've worked in and around srwlock for a long time. I don't think it's all that reasonable to think you should get "shared starve exclusive" by default, which is what you're asking for. We used to demand that kind of behavior in some windows filesystem code and it's always been a design mistake.
- LegionMammal978 2y ago
  I don't think SharedStarveExclusive is what GP is talking about? From the description, it sounds like currently, a shared acquire immediately after an exclusive release will get silently 'upgraded' to an exclusive acquire in some cases (and remain exclusive until released), which can easily cause issues. And it can't even be detected or worked around, since the SRWLock functions don't return any information about the status of the lock. It looks like the Rust standard library migrated away from SRWLOCK on account of this issue [0]. [0] https://github.com/rust-lang/rust/issues/121949 https://github.com/rust-lang/rust/issues/121949
  - tialaramex 2y ago
    Yeah, as that Rust issue states, this was first replicated by a C++ programmer and STL (the person, not the Microsoft name for the C++ stdlib) said it looks like a bug in SRWLock. There's a Microsoft internal ticket which I can't read (but STL can because he's a Microsoft employee) but there's also a GitHub issue for STL (this time the stdlib, a GitHub project, although it was opened by the person) https://github.com/microsoft/STL/issues/4448 https://github.com/microsoft/STL/issues/4448 and it's confirmed in that issue this is a bug in SRWLock, thus no work for STL† (either the project or the person). It's not unusual (especially in C++ but this happens anywhere) that people would rather contort themselves to believe crazy things than accept that it's just a bug. † Both Windows and C++ holds themselves to a very low standard, "Yeah, it's broken, too bad" is acceptable. Once you've determined that it's a bug you're done.
    - neerajsi 2y ago
      Wow, thanks for the links. I was previously unaware of this issue. I checked the referenced bug and I see that it is fixed and will be released in an upcoming version of Windows.