Skip to content

Commit 29ab321

Browse files
authored
fix: Cleanup provisional cycle head memos when query panics (#993)
* fix: Cleanup provisional cycle head memos on panic * Update test outputs
1 parent e257df1 commit 29ab321

11 files changed

+88
-33
lines changed

src/function/execute.rs

Lines changed: 58 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ use crate::function::{Configuration, IngredientImpl};
55
use crate::sync::atomic::{AtomicBool, Ordering};
66
use crate::tracked_struct::Identity;
77
use crate::zalsa::{MemoIngredientIndex, Zalsa, ZalsaDatabase};
8-
use crate::zalsa_local::ActiveQueryGuard;
8+
use crate::zalsa_local::{ActiveQueryGuard, QueryRevisions};
99
use crate::{Event, EventKind, Id};
1010

1111
impl<C> IngredientImpl<C>
@@ -141,6 +141,7 @@ where
141141
// only when a cycle is actually encountered.
142142
let mut opt_last_provisional: Option<&Memo<'db, C>> = None;
143143
let mut last_stale_tracked_ids: Vec<(Identity, Id)> = Vec::new();
144+
let _guard = ClearCycleHeadIfPanicking::new(self, zalsa, id, memo_ingredient_index);
144145

145146
loop {
146147
let previous_memo = opt_last_provisional.or(opt_old_memo);
@@ -210,6 +211,9 @@ where
210211
// `iteration_count` can't overflow as we check it against `MAX_ITERATIONS`
211212
// which is less than `u32::MAX`.
212213
iteration_count = iteration_count.increment().unwrap_or_else(|| {
214+
tracing::warn!(
215+
"{database_key_index:?}: execute: too many cycle iterations"
216+
);
213217
panic!("{database_key_index:?}: execute: too many cycle iterations")
214218
});
215219
zalsa.event(&|| {
@@ -222,10 +226,7 @@ where
222226
completed_query
223227
.revisions
224228
.update_iteration_count(iteration_count);
225-
crate::tracing::debug!(
226-
"{database_key_index:?}: execute: iterate again, revisions: {revisions:#?}",
227-
revisions = &completed_query.revisions
228-
);
229+
crate::tracing::info!("{database_key_index:?}: execute: iterate again...",);
229230
opt_last_provisional = Some(self.insert_memo(
230231
zalsa,
231232
id,
@@ -297,3 +298,55 @@ where
297298
(new_value, active_query.pop())
298299
}
299300
}
301+
302+
/// Replaces any inserted memo with a fixpoint initial memo without a value if the current thread panics.
303+
///
304+
/// A regular query doesn't insert any memo if it panics and the query
305+
/// simply gets re-executed if any later called query depends on the panicked query (and will panic again unless the query isn't deterministic).
306+
///
307+
/// Unfortunately, this isn't the case for cycle heads because Salsa first inserts the fixpoint initial memo and later inserts
308+
/// provisional memos for every iteration. Detecting whether a query has previously panicked
309+
/// in `fetch` (e.g., `validate_same_iteration`) and requires re-execution is probably possible but not very straightforward
310+
/// and it's easy to get it wrong, which results in infinite loops where `Memo::provisional_retry` keeps retrying to get the latest `Memo`
311+
/// but `fetch` doesn't re-execute the query for reasons.
312+
///
313+
/// Specifically, a Memo can linger after a panic, which is then incorrectly returned
314+
/// by `fetch_cold_cycle` because it passes the `shallow_verified_memo` check instead of inserting
315+
/// a new fix point initial value if that happens.
316+
///
317+
/// We could insert a fixpoint initial value here, but it seems unnecessary.
318+
struct ClearCycleHeadIfPanicking<'a, C: Configuration> {
319+
ingredient: &'a IngredientImpl<C>,
320+
zalsa: &'a Zalsa,
321+
id: Id,
322+
memo_ingredient_index: MemoIngredientIndex,
323+
}
324+
325+
impl<'a, C: Configuration> ClearCycleHeadIfPanicking<'a, C> {
326+
fn new(
327+
ingredient: &'a IngredientImpl<C>,
328+
zalsa: &'a Zalsa,
329+
id: Id,
330+
memo_ingredient_index: MemoIngredientIndex,
331+
) -> Self {
332+
Self {
333+
ingredient,
334+
zalsa,
335+
id,
336+
memo_ingredient_index,
337+
}
338+
}
339+
}
340+
341+
impl<C: Configuration> Drop for ClearCycleHeadIfPanicking<'_, C> {
342+
fn drop(&mut self) {
343+
if std::thread::panicking() {
344+
let revisions =
345+
QueryRevisions::fixpoint_initial(self.ingredient.database_key_index(self.id));
346+
347+
let memo = Memo::new(None, self.zalsa.current_revision(), revisions);
348+
self.ingredient
349+
.insert_memo(self.zalsa, self.id, memo, self.memo_ingredient_index);
350+
}
351+
}
352+
}

src/function/sync.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,9 +97,11 @@ impl ClaimGuard<'_> {
9797
syncs.remove(&self.key_index).expect("key claimed twice?");
9898

9999
if anyone_waiting {
100+
let database_key = DatabaseKeyIndex::new(self.sync_table.ingredient, self.key_index);
100101
self.zalsa.runtime().unblock_queries_blocked_on(
101-
DatabaseKeyIndex::new(self.sync_table.ingredient, self.key_index),
102+
database_key,
102103
if thread::panicking() {
104+
tracing::info!("Unblocking queries blocked on {database_key:?} after a panick");
103105
WaitResult::Panicked
104106
} else {
105107
WaitResult::Completed

src/runtime.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ impl Running<'_> {
9090
})
9191
});
9292

93-
crate::tracing::debug!(
93+
crate::tracing::info!(
9494
"block_on: thread {thread_id:?} is blocking on {database_key:?} in thread {other_id:?}",
9595
);
9696

tests/compile-fail/get-set-on-private-input-field.stderr

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
error[E0624]: method `field` is private
22
--> tests/compile-fail/get-set-on-private-input-field.rs:12:11
33
|
4-
2 | #[salsa::input]
4+
2 | #[salsa::input]
55
| --------------- private method defined here
66
...
77
12 | input.field(&db);
@@ -10,7 +10,7 @@ error[E0624]: method `field` is private
1010
error[E0624]: method `set_field` is private
1111
--> tests/compile-fail/get-set-on-private-input-field.rs:13:11
1212
|
13-
2 | #[salsa::input]
13+
2 | #[salsa::input]
1414
| --------------- private method defined here
1515
...
1616
13 | input.set_field(&mut db).to(23);

tests/compile-fail/input_struct_incompatibles.stderr

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ error: cannot find attribute `tracked` in this scope
5555
|
5656
help: consider importing one of these attribute macros
5757
|
58-
1 + use salsa::tracked;
58+
1 + use salsa::tracked;
5959
|
60-
1 + use salsa_macros::tracked;
60+
1 + use salsa_macros::tracked;
6161
|

tests/compile-fail/interned_struct_incompatibles.stderr

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ error: cannot find attribute `tracked` in this scope
4949
|
5050
help: consider importing one of these attribute macros
5151
|
52-
1 + use salsa::tracked;
52+
1 + use salsa::tracked;
5353
|
54-
1 + use salsa_macros::tracked;
54+
1 + use salsa_macros::tracked;
5555
|

tests/compile-fail/span-input-setter.stderr

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,10 @@ error[E0308]: mismatched types
1111
note: method defined here
1212
--> tests/compile-fail/span-input-setter.rs:3:5
1313
|
14-
1 | #[salsa::input]
14+
1 | #[salsa::input]
1515
| ---------------
16-
2 | pub struct MyInput {
17-
3 | field: u32,
16+
2 | pub struct MyInput {
17+
3 | field: u32,
1818
| ^^^^^
1919
help: consider mutably borrowing here
2020
|

tests/compile-fail/tracked_fn_return_not_update.stderr

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -7,18 +7,18 @@ error[E0369]: binary operation `==` cannot be applied to type `&NotUpdate`
77
note: an implementation of `PartialEq` might be missing for `NotUpdate`
88
--> tests/compile-fail/tracked_fn_return_not_update.rs:7:1
99
|
10-
7 | struct NotUpdate;
10+
7 | struct NotUpdate;
1111
| ^^^^^^^^^^^^^^^^ must implement `PartialEq`
1212
help: consider annotating `NotUpdate` with `#[derive(PartialEq)]`
1313
|
14-
7 + #[derive(PartialEq)]
15-
8 | struct NotUpdate;
14+
7 + #[derive(PartialEq)]
15+
8 | struct NotUpdate;
1616
|
1717

1818
error[E0599]: the function or associated item `maybe_update` exists for struct `UpdateDispatch<NotUpdate>`, but its trait bounds were not satisfied
1919
--> tests/compile-fail/tracked_fn_return_not_update.rs:10:56
2020
|
21-
7 | struct NotUpdate;
21+
7 | struct NotUpdate;
2222
| ---------------- doesn't satisfy `NotUpdate: PartialEq` or `NotUpdate: Update`
2323
...
2424
10 | fn tracked_fn<'db>(db: &'db dyn Db, input: MyInput) -> NotUpdate {
@@ -40,6 +40,6 @@ note: the trait `Update` must be implemented
4040
| ^^^^^^^^^^^^^^^^^^^^^^^
4141
help: consider annotating `NotUpdate` with `#[derive(PartialEq)]`
4242
|
43-
7 + #[derive(PartialEq)]
44-
8 | struct NotUpdate;
43+
7 + #[derive(PartialEq)]
44+
8 | struct NotUpdate;
4545
|

tests/compile-fail/tracked_impl_incompatibles.stderr

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ error: unexpected token
5555
error[E0119]: conflicting implementations of trait `Default` for type `MyTracked<'_>`
5656
--> tests/compile-fail/tracked_impl_incompatibles.rs:12:1
5757
|
58-
7 | impl<'db> std::default::Default for MyTracked<'db> {
58+
7 | impl<'db> std::default::Default for MyTracked<'db> {
5959
| -------------------------------------------------- first implementation here
6060
...
6161
12 | impl<'db> std::default::Default for MyTracked<'db> {
@@ -64,7 +64,7 @@ error[E0119]: conflicting implementations of trait `Default` for type `MyTracked
6464
error[E0119]: conflicting implementations of trait `Default` for type `MyTracked<'_>`
6565
--> tests/compile-fail/tracked_impl_incompatibles.rs:17:1
6666
|
67-
7 | impl<'db> std::default::Default for MyTracked<'db> {
67+
7 | impl<'db> std::default::Default for MyTracked<'db> {
6868
| -------------------------------------------------- first implementation here
6969
...
7070
17 | impl<'db> std::default::Default for MyTracked<'db> {
@@ -73,7 +73,7 @@ error[E0119]: conflicting implementations of trait `Default` for type `MyTracked
7373
error[E0119]: conflicting implementations of trait `Default` for type `MyTracked<'_>`
7474
--> tests/compile-fail/tracked_impl_incompatibles.rs:22:1
7575
|
76-
7 | impl<'db> std::default::Default for MyTracked<'db> {
76+
7 | impl<'db> std::default::Default for MyTracked<'db> {
7777
| -------------------------------------------------- first implementation here
7878
...
7979
22 | impl<'db> std::default::Default for MyTracked<'db> {
@@ -82,7 +82,7 @@ error[E0119]: conflicting implementations of trait `Default` for type `MyTracked
8282
error[E0119]: conflicting implementations of trait `Default` for type `MyTracked<'_>`
8383
--> tests/compile-fail/tracked_impl_incompatibles.rs:27:1
8484
|
85-
7 | impl<'db> std::default::Default for MyTracked<'db> {
85+
7 | impl<'db> std::default::Default for MyTracked<'db> {
8686
| -------------------------------------------------- first implementation here
8787
...
8888
27 | impl<'db> std::default::Default for MyTracked<'db> {
@@ -91,7 +91,7 @@ error[E0119]: conflicting implementations of trait `Default` for type `MyTracked
9191
error[E0119]: conflicting implementations of trait `Default` for type `MyTracked<'_>`
9292
--> tests/compile-fail/tracked_impl_incompatibles.rs:32:1
9393
|
94-
7 | impl<'db> std::default::Default for MyTracked<'db> {
94+
7 | impl<'db> std::default::Default for MyTracked<'db> {
9595
| -------------------------------------------------- first implementation here
9696
...
9797
32 | impl<'db> std::default::Default for MyTracked<'db> {
@@ -100,7 +100,7 @@ error[E0119]: conflicting implementations of trait `Default` for type `MyTracked
100100
error[E0119]: conflicting implementations of trait `Default` for type `MyTracked<'_>`
101101
--> tests/compile-fail/tracked_impl_incompatibles.rs:37:1
102102
|
103-
7 | impl<'db> std::default::Default for MyTracked<'db> {
103+
7 | impl<'db> std::default::Default for MyTracked<'db> {
104104
| -------------------------------------------------- first implementation here
105105
...
106106
37 | impl<'db> std::default::Default for MyTracked<'db> {
@@ -109,7 +109,7 @@ error[E0119]: conflicting implementations of trait `Default` for type `MyTracked
109109
error[E0119]: conflicting implementations of trait `Default` for type `MyTracked<'_>`
110110
--> tests/compile-fail/tracked_impl_incompatibles.rs:42:1
111111
|
112-
7 | impl<'db> std::default::Default for MyTracked<'db> {
112+
7 | impl<'db> std::default::Default for MyTracked<'db> {
113113
| -------------------------------------------------- first implementation here
114114
...
115115
42 | impl<'db> std::default::Default for MyTracked<'db> {
@@ -118,7 +118,7 @@ error[E0119]: conflicting implementations of trait `Default` for type `MyTracked
118118
error[E0119]: conflicting implementations of trait `Default` for type `MyTracked<'_>`
119119
--> tests/compile-fail/tracked_impl_incompatibles.rs:47:1
120120
|
121-
7 | impl<'db> std::default::Default for MyTracked<'db> {
121+
7 | impl<'db> std::default::Default for MyTracked<'db> {
122122
| -------------------------------------------------- first implementation here
123123
...
124124
47 | impl<'db> std::default::Default for MyTracked<'db> {

tests/compile-fail/tracked_struct_not_update.stderr

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,6 @@ note: the trait `Update` must be implemented
2929
= note: this error originates in the attribute macro `salsa::tracked` (in Nightly builds, run with -Z macro-backtrace for more info)
3030
help: consider annotating `NotUpdate` with `#[derive(PartialEq)]`
3131
|
32-
7 + #[derive(PartialEq)]
33-
8 | struct NotUpdate;
32+
7 + #[derive(PartialEq)]
33+
8 | struct NotUpdate;
3434
|

0 commit comments

Comments
 (0)