Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Rust's Sneaky Deadlock With `if let` Blocks

brooksblog.bearblog.dev

Post reply on HN
Print view View original

41–50 of 85 posts

Re: Rust's Sneaky Deadlock With `if let` Blocks

#41

Post by gmueckl »

GolDDranks wrote: post #36
Animats wrote: post #33

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.

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.

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.

Re: Rust's Sneaky Deadlock With `if let` Blocks

#42

Post by sunshowers »

acjohnson55 wrote: post #40
sunshowers wrote: post #19

Earlier quoted context omitted.

From my understanding: The original implementation is consistent with the behavior of `match`. However it was realized that it is both less intuitive than early dropping (as this post suggests) and also gets in the way of understandable semantics for if let chains, so the decision was made to change it in the next edition. match still retains the old behavior.

Why does the behavior make sense in match?

https://github.com/rust-lang/rust/issues/131154 has some discussion about all the possibilities. It's pretty subtle, but I think everyone agrees that the current behavior of if let is unintuitive. Beyond that there are a bunch of possibilities none of which is clearly superior.

Re: Rust's Sneaky Deadlock With `if let` Blocks

#43

Post by reitzensteinm »

Animats wrote: post #33

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.

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)

Re: Rust's Sneaky Deadlock With `if let` Blocks

#44

Post by GolDDranks »

gmueckl wrote: post #41
GolDDranks wrote: post #36

Earlier quoted context omitted.

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.

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.

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...

Re: Rust's Sneaky Deadlock With `if let` Blocks

#45

Post by GolDDranks »

acjohnson55 wrote: post #40
sunshowers wrote: post #19

Earlier quoted context omitted.

From my understanding: The original implementation is consistent with the behavior of `match`. However it was realized that it is both less intuitive than early dropping (as this post suggests) and also gets in the way of understandable semantics for if let chains, so the decision was made to change it in the next edition. match still retains the old behavior.

Why does the behavior make sense in match?

Match has multiple branches, and some or all of them can bind variables, (borrowed from the value matched against), defined in that branch. For that to be possible, it means that the temporary lifetime of the matched value must encompass all the branches.

Compared to that, if let - else has only two branches: one where matching (and possible variable binding) happens, and one where it doesn't. You couldn't have anything borrowed from the matched value in the else branch, so the lifetime extending to encompass the else branch is not strictly necessary.

Re: Rust's Sneaky Deadlock With `if let` Blocks

#46

Post by GolDDranks »

sunshowers wrote: post #42
acjohnson55 wrote: post #40

Earlier quoted context omitted.

Why does the behavior make sense in match?

https://github.com/rust-lang/rust/issues/131154 has some discussion about all the possibilities. It's pretty subtle, but I think everyone agrees that the current behavior of if let is unintuitive. Beyond that there are a bunch of possibilities none of which is clearly superior.

https://github.com/rust-lang/rust/issues/131154#issuecomment... Especially this matrix was helpful! The behaviour that's going to be adopted in Rust 2024 is the Rust2024.1 one. They were having doubts about the rules becoming too complex/non-uniform, but at least dropping early means compiler errors rather than deadlocks.

Re: Rust's Sneaky Deadlock With `if let` Blocks

#47

Post by colanderman »

Animats wrote: post #33

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.

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.

Re: Rust's Sneaky Deadlock With `if let` Blocks

#48

Post by ajross »

sunshowers wrote: post #16
dmitrygr wrote: post #2

This is why pthread_mutex_lock() and pthread_mutex_unlock() will always be kings - it is clear when things are locked and when they are unlocked. Nobody needs to write articles warning you about pthread_mutex being taken randomly by syntactic sugar and not released till a magic later time due to the same sugar. Ditto for RWlock/XYZloc/ABClock/etc...

I think mutexes that own their data are genuinely much easier to reason about than mutexes that don't. In this case this is a Rust bug that will be fixed in the next edition: https://github.com/rust-lang/rust/issues/124085

That's arguably true, but only insofar as it makes sense to talk about a mutex "owning" "data". There are many kinds of data that can't be owned by the language runtime like this (think about async/shared/DMA buffers, register blocks on foreign hardware, memory-mapped database files), but that still clearly need synchronization. Even things like coarse-grained locks taken at the subsystem level (e.g. to avoid having a separate atomic in every little object) are a bad fit for this kind of API.

There's value to being opinionated about API design, but there's also cost. And Rust is supposed to be playing in the same sandbox as the lower level tools.

Re: Rust's Sneaky Deadlock With `if let` Blocks

#49

Post by NobodyNada »

GolDDranks wrote: post #45
acjohnson55 wrote: post #40

Earlier quoted context omitted.

Why does the behavior make sense in match?

Match has multiple branches, and some or all of them can bind variables, (borrowed from the value matched against), defined in that branch. For that to be possible, it means that the temporary lifetime of the matched value must encompass all the branches. Compared to that, if let - else has only two branches: one where matching (and possible variable binding) happens, and one where it doesn't. You couldn't have anyth…

Also, from a pure syntax perspective, an if let block reads like it creates a binding that exists within the scope of the braces after the if, and a match block reads like it creates a binding that exists within the "scope" of the braces surrounding the entire match block.

These all read the same to me from a scoping perspective:

    if let Some(x) = expr {
        // expr is live here
    }

    while let Some(x) = expr {
        // expr is live here
    }

    match expr {
        // expr is live here
        Some(x) => {},
        _ => { /* expr is still live, doesn't matter that x is inaccessible */
    }

    fn foo(x: i32) {
        // same idea, the variable x belongs to the scope it's declared alongside
    }
For the scope of an if-let expression to extend into the else block afterwards violates the principle of least surprise for me -- and clearly the author of the OP too!

    if let Some(x) = expr {
        // expr is live here
    } else {
        // ??? why is expr live here? the if is clearly "out of scope!"
    }

Re: Rust's Sneaky Deadlock With `if let` Blocks

#50

Post by Animats »

gmueckl wrote: post #41
GolDDranks wrote: post #36

Earlier quoted context omitted.

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.

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.

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.

Post reply on HN