Implement "opinionated cancellation" #265

jonas-schievink · 2021-05-17T17:00:56Z

This implements the design described in RFC #262.

Currently, the in_par_get_set_cancellation test is failing. I'm not completely sure what it is trying to test (the comment on the test does not match its behavior), so I couldn't fix it.

jonas-schievink · 2021-05-17T17:08:16Z

rust-analyzer companion PR: rust-lang/rust-analyzer#8866

src/lib.rs

matklad · 2021-05-17T17:16:55Z

src/lib.rs

+    }
+}
+
+impl std::error::Error for Canceled {}


I think we should have this, but this was not obvious to me, so let me write down justification.

I would expect Cancellation to not be treated as an error. Cancellation is a serendipitous success. It would be wrong to propagate Cancelled as Error. However, in salsa itself Cancelled is not used throughout the API, as we use unwinding, its used only at the boundady. For boundaries, I think treating it as an Error is probably fine

src/lib.rs

src/runtime.rs

src/lib.rs

matklad · 2021-05-17T18:55:10Z

Apparently, one L is the American variant, and I think Rust in general favors US speling, so we should do that.

…

On Mon, 17 May 2021 at 21:51, Laurențiu Nicola ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/lib.rs <#265 (comment)>: > @@ -644,6 +645,40 @@ where } } +/// A panic payload indicating that a salsa revision was canceled. +#[derive(Debug)] +pub struct Canceled { Nit: maybe call this Cancelled to match the test filename? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#265 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANB3MY3UFTK4CHM6EQNCDDTOFQUXANCNFSM45A3BNEA> .

lnicola · 2021-05-17T18:59:19Z

Yeah, but there's also the more American "cancelation" which matches "canceled", while the code used "cancellation". TBH, the American spelling hurts my eyes 😅.

src/derived/slot.rs

src/runtime.rs

matklad · 2021-05-18T13:33:01Z

src/runtime.rs

@@ -38,6 +41,8 @@ pub struct Runtime {

    /// Shared state that is accessible via all runtimes.
    shared_state: Arc<SharedState>,
+
+    on_cancelation_check: Option<Box<dyn Fn() + RefUnwindSafe + Send>>,


Hm, why do we need this? I imagined this can work the same way as other events, by user overriding on_salsa_event or some such.

salsa/src/lib.rs

Lines 67 to 73 in b3cb3c4

/// This function is invoked at key points in the salsa

/// runtime. It permits the database to be customized and to

/// inject logging or other custom behavior.

fn salsa_event(&self, event_fn: Event) {

#![allow(unused_variables)]

}

That needs a Database, which unwind_if_canceled doesn't have. Should we pass the db as an argument here?

Hm, perhaps we should make unwind_if_canceled a method of the Database then? That would actually give a nice way to opt-out of unwinding -- overrride it to be no-op. Not sure if I love or hate that flexibility though.

nikomatsakis

This looks basically good to me -- I wasn't clear @jonas-schievink on whether there are broken tests? I guess I can check CI :

src/derived/slot.rs

nikomatsakis · 2021-05-18T19:32:19Z

src/runtime.rs

-            assert_eq!(pending_revision, current_revision);
-            self.report_anon_read(pending_revision);
-            false
+            self.unwind_canceled();


This simplification is quite nice.

I'm a bit worried that there could be a subtle bug if we allowed users to override cancelation here.

nikomatsakis · 2021-05-18T19:33:35Z

src/runtime.rs

+
+    /// Registers a callback to be invoked every time [`Runtime::unwind_if_canceled`] is called
+    /// (either automatically by salsa, or manually by user code).
+    pub fn set_cancelation_check_callback<F>(&mut self, callback: F)


I don't know why, but "cancellation" is far more common...

I agree with @matklad that it would be nice to do this via salsa_event -- do we have a concrete use for this?

The primary use case is to assert that no more than N milliseconds pass between cancellation checks, which is helpful for debugging long-running queries that affect rust-analyzer responsiveness

nikomatsakis · 2021-05-18T19:40:41Z

can you try running the tests with RUST_BACKTRACE=1? I'm confused where the error originates :(

jonas-schievink · 2021-05-19T09:54:42Z

Test failure backtrace:

thread 'frozen::in_par_get_set_cancelation' panicked at 'called `Result::unwrap()` on an `Err` value: Any { .. }', tests/parallel/frozen.rs:64:33
stack backtrace:
   0: rust_begin_unwind
             at /rustc/881c1ac408d93bb7adaa3a51dabab9266e82eee8/library/std/src/panicking.rs:493:5
   1: core::panicking::panic_fmt
             at /rustc/881c1ac408d93bb7adaa3a51dabab9266e82eee8/library/core/src/panicking.rs:92:14
   2: core::result::unwrap_failed
             at /rustc/881c1ac408d93bb7adaa3a51dabab9266e82eee8/library/core/src/result.rs:1355:5
   3: core::result::Result<T,E>::unwrap
             at /home/jonas/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/result.rs:1037:23
   4: parallel::frozen::in_par_get_set_cancelation
             at ./tests/parallel/frozen.rs:64:18
   5: parallel::frozen::in_par_get_set_cancelation::{{closure}}
             at ./tests/parallel/frozen.rs:13:1
   6: core::ops::function::FnOnce::call_once
             at /home/jonas/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:227:5
   7: core::ops::function::FnOnce::call_once
             at /rustc/881c1ac408d93bb7adaa3a51dabab9266e82eee8/library/core/src/ops/function.rs:227:5

nikomatsakis · 2021-05-21T15:59:09Z

@jonas-schievink hmm that's not very useful. I was hoping for a backtrace more in the spawned threads. I'll try to run myself.

nikomatsakis · 2021-05-21T16:38:29Z

OK, so the error seeems-- ah, I know what's going on. Maybe it was obvious to you, @jonas-schievink, but it wasn't to me. What is happening is that the call to db.input('a') in the first thread is now checking the cancellation flag and hence you get cancellation. Before we were intentionally swallowing it, which you can no longer do. I think this test can just be modified to remove the logic after the catch_unwind code entirely?

nikomatsakis

This looks great!

nikomatsakis · 2021-05-27T10:59:12Z

src/lib.rs

+    /// Cancellation will automatically be triggered by salsa on any query
+    /// invocation.
+    ///
+    /// This method should not be overridden by `Database` implementors.


We should mention that it invokes salsa_event with WillCheckCancellation, so implementors can override that to intercept.

nikomatsakis · 2021-05-27T11:00:04Z

Oh, I guess I should have marked that as "Request changes". Apart from the documentation nit, r=me

matklad · 2021-05-27T12:30:04Z

bors r=nikomatsakis

bors · 2021-05-27T12:31:53Z

Build succeeded:

Test (stable, false)

- new cancellation API salsa-rs#265

8866: Update salsa r=matklad a=jonas-schievink This updates salsa to include salsa-rs/salsa#265, and removes all cancellation-related code from rust-analyzer Co-authored-by: Jonas Schievink <jonasschievink@gmail.com>

Implement "opinionated cancellation"

197b01f

jonas-schievink mentioned this pull request May 17, 2021

Update salsa rust-lang/rust-analyzer#8866

Merged

matklad reviewed May 17, 2021

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

lnicola reviewed May 17, 2021

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

jonas-schievink added 3 commits May 18, 2021 14:41

Improve Canceled API

49a1184

Add a cancelation check callback to the runtime

5ac74ab

Remove on_propagated_panic

877d3f2

matklad reviewed May 18, 2021

View reviewed changes

jonas-schievink added 2 commits May 18, 2021 15:36

Canonicalize to US spelling of "cancelation"

223f87b

Retain original import style

bb4b42a

nikomatsakis reviewed May 18, 2021

View reviewed changes

Explain why we don't duplicate panics from other threads

018658b

jonas-schievink added 3 commits May 25, 2021 14:55

Fix "frozen" test

f9cb032

Use the more common spelling of cancell{ed,ation}

1fb660c

Move unwind_if_cancelled to Database

458266e

jonas-schievink marked this pull request as ready for review May 25, 2021 13:55

nikomatsakis approved these changes May 27, 2021

View reviewed changes

Mention that salsa_event should be used instead

b9fc829

bors bot merged commit e17e77c into salsa-rs:master May 27, 2021

jonas-schievink deleted the opinionated-cancellation branch May 27, 2021 12:33

matklad added a commit to matklad/salsa that referenced this pull request May 29, 2021

Publish 0.17.0-pre.1

a84ee62

- new cancellation API salsa-rs#265

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement "opinionated cancellation" #265

Implement "opinionated cancellation" #265

jonas-schievink commented May 17, 2021

jonas-schievink commented May 17, 2021

matklad May 17, 2021

matklad commented May 17, 2021 via email

lnicola commented May 17, 2021

matklad May 18, 2021

matklad May 18, 2021

jonas-schievink May 18, 2021

matklad May 18, 2021

nikomatsakis left a comment

nikomatsakis May 18, 2021

nikomatsakis May 18, 2021

nikomatsakis May 18, 2021

nikomatsakis May 18, 2021

jonas-schievink May 18, 2021

nikomatsakis commented May 18, 2021

jonas-schievink commented May 19, 2021

nikomatsakis commented May 21, 2021

nikomatsakis commented May 21, 2021

nikomatsakis left a comment

nikomatsakis May 27, 2021

nikomatsakis commented May 27, 2021

matklad commented May 27, 2021

bors bot commented May 27, 2021

	/// This function is invoked at key points in the salsa
	/// runtime. It permits the database to be customized and to
	/// inject logging or other custom behavior.
	fn salsa_event(&self, event_fn: Event) {
	#![allow(unused_variables)]
	}

Implement "opinionated cancellation" #265

Implement "opinionated cancellation" #265

Conversation

jonas-schievink commented May 17, 2021

jonas-schievink commented May 17, 2021

Choose a reason for hiding this comment

matklad commented May 17, 2021 via email

lnicola commented May 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented May 18, 2021

jonas-schievink commented May 19, 2021

nikomatsakis commented May 21, 2021

nikomatsakis commented May 21, 2021

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented May 27, 2021

matklad commented May 27, 2021

bors bot commented May 27, 2021