2 ms·

A lock taken in the conditional test term should lock for the duration of the if-clause. If you release the lock early, you add the risk of a race condition, wh

A lock taken in the conditional test term should lock for the duration of the if-clause. If you release the lock early, you add the risk of a race condition, where some other thread executes between the test and the execution of the if or else clause, changing the state.

GolDDranks 2y ago
But in Rust, the lock protects the contents inside it and lends the contents only for the lifetime the lock is being held, so if one tries to modify the contents when the lock is already released, you are going to get a compiler error, not a race condition.
- gmueckl 2y ago
  Parent was referring to a time of check to time of use bug. This kind of problem occurs every time you need to release and reacquire a lock between the point of decision and the point where the data is modified. The decision on which the modification is based may have been invalidated by another thread while the lock was released.
  - GolDDranks 2y ago
    I was going to say "d'oh, that's locking 101", but the original article indeed does that kind of a thing with the else branch. They should have chosen a better example do demonstrate the point, but it's hard to say if the current racy behaviour is even wrong because they didn't have any real-life semantics/use-case in the example...
  - Animats 2y ago
    Right. I could have been clearer. There are two errors to avoid here: - Read lock, read some data, unlock, relock for writing, act on data read during previous locked period. That's a time of check/time of use error. - Acquire a read lock, then upgrade to a write lock. MutexLock does not let you do that. If you do that from two threads, you can deadlock. You can do that in SQL, but SQL can back out a transaction that deadlocks. This is not a problem with Rust. Rust prevented the author from shooting themself in the foot.
reitzensteinm 2y ago
Right - if the pattern in the referenced blog post is used to implement lazy init, racing threads can double initialize. But it's not possible to upgrade a read to write lock as racing threads can cause a deadlock, and upgradable RwLock variants don't hand out multiple upgradable read locks. To maintain concurrent access, the referenced code must recheck the data once a write lock is granted, to gracefully handle TOCTOU. (I know you were making a general point, I'm just fleshing out this specific example)
colanderman 2y ago
This is true, but it's also true that a lock ought only be held in the lexical scope of its guard's let binding. That the scope of temporaries is extended outside the binding scope in this one case is surprising. In a review I'd flag code which depended on this behavior -- it's much clearer instead to simply take the lock outside the if statement.
kelnos 2y ago
Right. But the problem is that with an 'if let', it also locks for the 'else', which is not intuitive or expected. The problem is that the syntax of 'if let' makes the programmer assume a particular scoping, but its desugaring implies something else.

GolDDranks 2y ago

But in Rust, the lock protects the contents inside it and lends the contents only for the lifetime the lock is being held, so if one tries to modify the contents when the lock is already released, you are going to get a compiler error, not a race condition.

gmueckl 2y ago
Parent was referring to a time of check to time of use bug. This kind of problem occurs every time you need to release and reacquire a lock between the point of decision and the point where the data is modified. The decision on which the modification is based may have been invalidated by another thread while the lock was released.
- GolDDranks 2y ago
  I was going to say "d'oh, that's locking 101", but the original article indeed does that kind of a thing with the else branch. They should have chosen a better example do demonstrate the point, but it's hard to say if the current racy behaviour is even wrong because they didn't have any real-life semantics/use-case in the example...
- Animats 2y ago
  Right. I could have been clearer. There are two errors to avoid here: - Read lock, read some data, unlock, relock for writing, act on data read during previous locked period. That's a time of check/time of use error. - Acquire a read lock, then upgrade to a write lock. MutexLock does not let you do that. If you do that from two threads, you can deadlock. You can do that in SQL, but SQL can back out a transaction that deadlocks. This is not a problem with Rust. Rust prevented the author from shooting themself in the foot.

reitzensteinm 2y ago

Right - if the pattern in the referenced blog post is used to implement lazy init, racing threads can double initialize. But it's not possible to upgrade a read to write lock as racing threads can cause a deadlock, and upgradable RwLock variants don't hand out multiple upgradable read locks. To maintain concurrent access, the referenced code must recheck the data once a write lock is granted, to gracefully handle TOCTOU. (I know you were making a general point, I'm just fleshing out this specific example)

colanderman 2y ago

This is true, but it's also true that a lock ought only be held in the lexical scope of its guard's let binding. That the scope of temporaries is extended outside the binding scope in this one case is surprising. In a review I'd flag code which depended on this behavior -- it's much clearer instead to simply take the lock outside the if statement.

kelnos 2y ago

Right. But the problem is that with an 'if let', it also locks for the 'else', which is not intuitive or expected. The problem is that the syntax of 'if let' makes the programmer assume a particular scoping, but its desugaring implies something else.