awslabs / awslabs/shuttle

Early stopping doesn't work if a Drop impl tries to acquire locks

Open
#22 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
1.1k
Forks
59
Avg merge
4d 2h
Merged PRs (30d)
15

Description

This test panics in the `Drop` impl of `PoolItem`:

```rust
#[test]
fn max_steps_continue_lock_during_drop() {
let mut config = Config::new();
config.max_steps = MaxSteps::ContinueAfter(10);
let scheduler = DfsScheduler::new(None, false);
let runner = Runner::new(scheduler, config);
runner.run(|| {
#[derive(Clone)]
struct Pool {
items: Arc>>,
}

struct PoolItem {
pool: Arc>>,
item: usize,
}

impl Pool {
fn new(length: usize) -> Self {
Self {
items: Arc::new(Mutex::new((0..length).collect())),
}
}

fn get(&self) -> Option {
let mut items = self.items.lock().unwrap();
let item = items.pop_front()?;
Some(PoolItem {
pool: self.items.clone(),
item,
})
}
}

impl Drop for PoolItem {
fn drop(&mut self) {
let mut items = self.pool.lock().unwrap();
items.push_back(self.item);
}
}

let pool = Pool::new(10);

let threads: Vec<_> = (0..3).map(|_| {
let pool = pool.clone();
thread::spawn(move || {
let _item = pool.get();
thread::yield_now();
})
}).collect();

for thd in threads {
thd.join().unwrap();
}
})
}
```

The problem is that `ContinueAfter` makes us stop the test early, while some `PoolItem`s are still alive. During cleanup we drop the `Generator` for each thread, which tries to safely clean up by unwinding their stacks, including dropping the `PoolItem`. But `PoolItem`'s drop tries to acquire a lock, which is no longer allowed once `ContinueAfter` has triggered.

I'm not totally sure what to do here:
* We could just leak the continuation's stack...
* We could do something smarter during teardown, allowing threads to continue running until termination or deadlock, under the assumption that the only code that can run once we drop the continuations is cleanup code

Contributor guide

Open the contributing guide

Research direction

Start with the max_steps_continue_lock_during_drop test and trace Runner::run, MaxSteps::ContinueAfter, and Generator cleanup. The issue presents two possible teardown strategies but does not establish which invariant is required, so confirm that decision before changing behavior. Done should include a regression test showing cleanup of a Drop implementation that acquires a lock no longer panics.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
testing-qa
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.