use computed recovery strategy

Rather than checking return value of from `Q::cycle_fallback`, we
now consult the computed recovery strategy to decide whether to
panic or to recover. We can thus assume that we will successfully
recover and don't need to check for `None` results anymore.

components/salsa-macros/src/query_group.rs

@@ -486,12 +486,12 @@ pub(crate) fn query_group(args: TokenStream, input: TokenStream) -> TokenStream
                     const CYCLE_STRATEGY: salsa::plumbing::CycleRecoveryStrategy =
                         salsa::plumbing::CycleRecoveryStrategy::Fallback;
                     fn cycle_fallback(db: &<Self as salsa::QueryDb<'_>>::DynDb, cycle: &[salsa::DatabaseKeyIndex], #key_pattern: &<Self as salsa::Query>::Key)
-                        -> Option<<Self as salsa::Query>::Value> {
-                        Some(#cycle_recovery_fn(
+                        -> <Self as salsa::Query>::Value {
+                        #cycle_recovery_fn(
                                 db,
                                 &cycle.iter().map(|k| format!("{:?}", k.debug(db))).collect::<Vec<String>>(),
                                 #(#key_names),*
-                        ))
+                        )
                     }
                 }
             } else {

src/derived/slot.rs

@@ -4,7 +4,7 @@ use crate::derived::MemoizationPolicy;
 use crate::durability::Durability;
 use crate::lru::LruIndex;
 use crate::lru::LruNode;
-use crate::plumbing::CycleDetected;
+use crate::plumbing::{CycleDetected, CycleRecoveryStrategy};
 use crate::plumbing::{DatabaseOps, QueryFunction};
 use crate::revision::Revision;
 use crate::runtime::Runtime;
@@ -217,18 +217,7 @@ where
         });
 
         if !result.cycle.is_empty() {
-            result.value = match Q::cycle_fallback(db, &result.cycle, &self.key) {
-                Some(v) => v,
-                None => {
-                    let err = CycleError {
-                        cycle: result.cycle,
-                        durability: result.durability,
-                        changed_at: result.changed_at,
-                    };
-                    panic_guard.report_unexpected_cycle();
-                    return Err(err);
-                }
-            };
+            result.value = Q::cycle_fallback(db, &result.cycle, &self.key);
         }
 
         // We assume that query is side-effect free -- that is, does
@@ -357,6 +346,10 @@ where
                             // the panic hook and bubble up to the thread boundary (or be caught).
                             Cancelled::throw()
                         });
+                        debug!(
+                            "received result from other thread: cycle = {:?}",
+                            result.cycle
+                        );
                         ProbeState::UpToDate(if result.cycle.is_empty() {
                             Ok(result.value)
                         } else {
@@ -366,33 +359,29 @@ where
                                 durability: result.value.durability,
                             };
                             runtime.mark_cycle_participants(&err);
-                            Q::cycle_fallback(db, &err.cycle, &self.key)
-                                .map(|value| StampedValue {
-                                    value,
-                                    durability: err.durability,
-                                    changed_at: err.changed_at,
-                                })
-                                .ok_or(err)
+                            Ok(StampedValue {
+                                value: Q::cycle_fallback(db, &err.cycle, &self.key),
+                                durability: err.durability,
+                                changed_at: err.changed_at,
+                            })
                         })
                     }
 
-                    Err(err) => {
-                        let err = runtime.report_unexpected_cycle(
+                    Err(err) => ProbeState::UpToDate(
+                        match runtime.report_unexpected_cycle(
                             db.ops_database(),
                             self.database_key_index,
                             err,
                             revision_now,
-                        );
-                        ProbeState::UpToDate(
-                            Q::cycle_fallback(db, &err.cycle, &self.key)
-                                .map(|value| StampedValue {
-                                    value,
-                                    changed_at: err.changed_at,
-                                    durability: err.durability,
-                                })
-                                .ok_or(err),
-                        )
-                    }
+                        ) {
+                            (CycleRecoveryStrategy::Panic, err) => Err(err),
+                            (CycleRecoveryStrategy::Fallback, err) => Ok(StampedValue {
+                                value: Q::cycle_fallback(db, &err.cycle, &self.key),
+                                changed_at: err.changed_at,
+                                durability: err.durability,
+                            }),
+                        },
+                    ),
                 };
             }
 
@@ -677,9 +666,17 @@ where
     ) -> Result<BlockingFuture<WaitResult<Q::Value, DatabaseKeyIndex>>, CycleDetected> {
         let id = runtime.id();
         if other_id == id {
+            debug!(
+                "register_with_in_progress_thread: runtime {:?} cannot block on self",
+                id
+            );
             Err(CycleDetected { from: id, to: id })
         } else {
             if !runtime.try_block_on(self.database_key_index, other_id) {
+                debug!(
+                    "register_with_in_progress_thread: runtime {:?} cannot block on {:?}, cycle detected",
+                    id, other_id
+                );
                 return Err(CycleDetected {
                     from: id,
                     to: other_id,
@@ -692,6 +689,11 @@ where
             // lock, we don't need any particular ordering.
             waiting.lock().push(promise);
 
+            debug!(
+                "register_with_in_progress_thread: runtime {:?} blocked on {:?}",
+                id, other_id
+            );
+
             Ok(future)
         }
     }
@@ -751,11 +753,6 @@ where
         std::mem::forget(self)
     }
 
-    fn report_unexpected_cycle(mut self) {
-        self.overwrite_placeholder(None);
-        std::mem::forget(self)
-    }
-
     /// Overwrites the `InProgress` placeholder for `key` that we
     /// inserted; if others were blocked, waiting for us to finish,
     /// then notify them.

src/plumbing.rs

@@ -82,9 +82,12 @@ pub trait QueryFunction: Query {
         db: &<Self as QueryDb<'_>>::DynDb,
         cycle: &[DatabaseKeyIndex],
         key: &Self::Key,
-    ) -> Option<Self::Value> {
+    ) -> Self::Value {
         let _ = (db, cycle, key);
-        None
+        panic!(
+            "query `{:?}` doesn't support cycle fallback",
+            Self::default()
+        )
     }
 }
 

src/runtime.rs

@@ -292,7 +292,7 @@ impl Runtime {
         database_key_index: DatabaseKeyIndex,
         error: CycleDetected,
         changed_at: Revision,
-    ) -> crate::CycleError<DatabaseKeyIndex> {
+    ) -> (CycleRecoveryStrategy, crate::CycleError<DatabaseKeyIndex>) {
         debug!(
             "report_unexpected_cycle(database_key={:?})",
             database_key_index
@@ -304,11 +304,14 @@ impl Runtime {
             "cycle recovery strategy {:?} for participants {:?}",
             crs, cycle
         );
-        CycleError {
-            cycle,
-            changed_at,
-            durability: Durability::MAX,
-        }
+        (
+            crs,
+            CycleError {
+                cycle,
+                changed_at,
+                durability: Durability::MAX,
+            },
+        )
     }
 
     fn mutual_cycle_recovery_strategy(