huggingface / huggingface/candle
potential deadlock risk from multiple read locks over storage in tensor operations
- Dominant language
- Rust
- Stars
- 21k
- Forks
- 1.8k
- Avg merge
- 16h 42m
- Merged PRs (30d)
- 25
Description
Multiple tensor operations (for example `binary_op`) take two (or more) read locks over underlying storage that can deadlock if the `self `and `rhs`/`kernel` "other" storages are the same and there is a concurrent write lock request on the same storage. [std-sync RwLock](https://doc.rust-lang.org/std/sync/struct.RwLock.html) can deadlock two reads on the same thread if there is a concurrent write.
Since `storage()` takes a read lock and returns the guard
https://github.com/huggingface/candle/blob/6f74e7c390c717f8fd34f23ce02aceb058173370/candle-core/src/tensor.rs#L2741-L2743
for the following example from `binary_op`
https://github.com/huggingface/candle/blob/6f74e7c390c717f8fd34f23ce02aceb058173370/candle-core/src/tensor.rs#L101-L102
when `self `and `rhs` are the same, the underlying storage and its lock would be the same.
There is a similar reported issue https://github.com/huggingface/candle/issues/3227 that shows deadlock without concurrent writes, these are included here too but are a different kind of bug.
The deadlocks with no concurrency in **custom_op**:
| Function | Guards |
|---|---|
| `Tensor::scatter_set` | `self`[W] → `indexes`[R], `source`[R] |
| `Tensor::scatter_add_set` | `self`[W] → `indexes`[R], `source`[R] |
| `Tensor::inplace_op2` | `self`[W] → `rhs`[R] |
| `Tensor::inplace_op3` | `self`[W] → `t2`[R], `t3`[R] |
`scatter_set` and `scatter_add_set` already guard against source and self being the same, but not against indexes also being the same (seems like the "same storage" check is for correctness rather than deadlock prevention here). `Tensor::slice_set` and `Var::set` already check if they have the same storage and bail if true.
Deadlocks can occur if there are concurrent writes between the `.storage()` calls over the following variables:
In **tensor.rs**:
| Function | Overlapping |
|---|---|
| `add`, `sub`, `mul`, `div` | `self` → `rhs` |
| `cmp` | `self` → `rhs` |
| `matmul` | `self` → `rhs` |
| `where_cond` | `self`, `on_true`, `on_false` |
| `scatter` | `indexes` → `source` |
| `scatter_add` | `indexes` → `source` |
| `index_add` | `self`, `indexes`, `source` |
| `gather` | `self` → `indexes` |
| `index_select` | `self` → `indexes` |
In **conv.rs**, all `self` → `kernel`:
| Function |
|---|
| `conv1d_single_group` |
| `conv_transpose1d_single_group` |
| `conv2d_single_group` |
| `conv_transpose2d` |
In **custom_op.rs**:
| Function | Overlapping |
|---|---|
| `apply_op2_no_bwd` | `self` → `rhs` |
| `apply_op3_no_bwd` | `self`, `t2`, `t3` |
| `apply_op2_arc` | `self` → `rhs` |
| `apply_op3_arc` | `self`, `t2`, `t3` |
These possible deadlocks were found by a static analysis tool. There could be more deadlocks hiding behind `storage_and_layout()`.
The fix should check whether the two storages are the same before attempting to lock the second one or switch over to a reentrant RwLock like the one in parking_lot.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start in candle-core/src/tensor.rs with storage(), binary_op, and the listed tensor operations, then inspect the corresponding functions in conv.rs and custom_op.rs. Compare their lock ordering with the existing same-storage checks in Tensor::slice_set and Var::set, and trace storage_and_layout(). Done means the listed overlapping-storage cases no longer risk deadlock, including concurrent-write scenarios.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- machine-learning
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100