Skip to content

Commit 2f9dd6d

Browse files
committed
Revert "Fix a schedule() race by postponing preemption enablement to inside the dispatch closure"
This reverts commit cb51897, which was causing even more severe memory corruption, as evidenced by, e.g.,: $ cargo test -- --test-threads 1 --exact launch_union
1 parent 655d9f1 commit 2f9dd6d

File tree

2 files changed

+14
-15
lines changed

2 files changed

+14
-15
lines changed

src/linger.rs

Lines changed: 13 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,6 @@ struct Task {
9898
pub fn launch<T: Send>(fun: impl FnOnce() -> T + Send, us: u64)
9999
-> Result<Linger<T, impl FnMut(*mut Option<ThdResult<T>>) + Send>> {
100100
use groups::assign_group;
101-
use preemption::enable_preemption;
102101
use std::panic::AssertUnwindSafe;
103102
use std::panic::catch_unwind;
104103

@@ -124,22 +123,13 @@ pub fn launch<T: Send>(fun: impl FnOnce() -> T + Send, us: u64)
124123
// arising from the former case, we first assert that no one has attempted a nested call.
125124
debug_assert!(! is_preemptible(), "launch(): called from preemptible function");
126125

127-
let group = assign_group().expect("launch(): too many active timed functions");
128-
let group_id = *group;
129126
let mut fun = Completion::Function(AssertUnwindSafe (fun));
130127
let fun = Box::new(move |ret: *mut Option<ThdResult<T>>| {
131128
fun = match fun.take() {
132-
Completion::Function(fun) => {
129+
Completion::Function(fun) =>
133130
// We haven't yet moved the closure. This means schedule() is invoking us
134131
// as a *nullary* function, implying ret is undefined and mustn't be used.
135-
let res = catch_unwind(|| {
136-
stamp();
137-
enable_preemption(group_id.into());
138-
fun()
139-
});
140-
disable_preemption();
141-
Completion::Return(res)
142-
},
132+
Completion::Return(catch_unwind(fun)),
143133
Completion::Return(val) => {
144134
debug_assert!(! ret.is_null());
145135
let ret = unsafe {
@@ -154,6 +144,7 @@ pub fn launch<T: Send>(fun: impl FnOnce() -> T + Send, us: u64)
154144
});
155145

156146
let checkpoint = setup_stack()?;
147+
let group = assign_group().expect("launch(): too many active timed functions");
157148
let mut linger = Linger::Continuation(Continuation {
158149
functional: fun,
159150
stateful: Task {
@@ -170,7 +161,7 @@ pub fn launch<T: Send>(fun: impl FnOnce() -> T + Send, us: u64)
170161
}
171162

172163
thread_local! {
173-
static BOOTSTRAP: Cell<Option<NonNull<(dyn FnMut() + Send)>>> = Cell::default();
164+
static BOOTSTRAP: Cell<Option<(NonNull<(dyn FnMut() + Send)>, Group)>> = Cell::default();
174165
static TASK: RefCell<Task> = RefCell::default();
175166
static DEADLINE: Cell<u64> = Cell::default();
176167
}
@@ -201,7 +192,9 @@ pub fn resume<T>(fun: &mut Linger<T, impl FnMut(*mut Option<ThdResult<T>>) + Sen
201192
// represents our first caller, so we know not to read the argument.
202193
let fun: *mut (dyn FnMut(_) + Send) = fun;
203194
let fun: *mut (dyn FnMut() + Send) = fun as _;
204-
let no_fun = bootstrap.replace(NonNull::new(fun));
195+
let no_fun = bootstrap.replace(NonNull::new(fun).map(|fun|
196+
(fun, group)
197+
));
205198
debug_assert!(
206199
no_fun.is_none(),
207200
"resume(): bootstraps without an intervening schedule()",
@@ -336,13 +329,18 @@ fn switch_stack(task: &mut Task, group: Group) -> Result<bool> {
336329

337330
/// Enable preemption and call the preemptible function. Runs on the oneshot execution stack.
338331
fn schedule() {
339-
let mut fun = BOOTSTRAP.with(|bootstrap| bootstrap.take()).unwrap_or_else(||
332+
use preemption::enable_preemption;
333+
334+
let (mut fun, group) = BOOTSTRAP.with(|bootstrap| bootstrap.take()).unwrap_or_else(||
340335
abort("schedule(): called without bootstrapping")
341336
);
342337
let fun = unsafe {
343338
fun.as_mut()
344339
};
340+
stamp();
341+
enable_preemption(group.into());
345342
fun();
343+
disable_preemption();
346344

347345
// It's important that we haven't saved any errno, since we'll check it to determine whether
348346
// the preemptible function ran to completion.

tests/inger/main.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,7 @@ fn launch_continuations() {
8585
});
8686
}
8787

88+
#[ignore]
8889
#[test]
8990
fn resume_completion() {
9091
exclusive(|| {

0 commit comments

Comments
 (0)