From 2b8ee2043d7cac338efcaeb65ea649b584f5e65f Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Sat, 15 May 2021 05:32:37 -0400 Subject: [PATCH 1/5] add RFC: Opinionated cancelation --- book/src/SUMMARY.md | 3 +- .../rfcs/RFC0007-Opinionated-Cancelation.md | 63 +++++++++++++++++++ 2 files changed, 65 insertions(+), 1 deletion(-) create mode 100644 book/src/rfcs/RFC0007-Opinionated-Cancelation.md diff --git a/book/src/SUMMARY.md b/book/src/SUMMARY.md index 547fa37c..e81ac29c 100644 --- a/book/src/SUMMARY.md +++ b/book/src/SUMMARY.md @@ -18,4 +18,5 @@ - [RFC 0003: Query dependencies](./rfcs/RFC0003-Query-Dependencies.md) - [RFC 0004: LRU](./rfcs/RFC0004-LRU.md) - [RFC 0005: Durability](./rfcs/RFC0005-Durability.md) - - [RFC 0006: Dynamic database](./rfcs/RFC0006-Dynamic-Databases.md) \ No newline at end of file + - [RFC 0006: Dynamic database](./rfcs/RFC0006-Dynamic-Databases.md) + - [RFC 0007: Opinionated cancelation](./rfcs/RFC0007-Opinionated-Cancelation.md) \ No newline at end of file diff --git a/book/src/rfcs/RFC0007-Opinionated-Cancelation.md b/book/src/rfcs/RFC0007-Opinionated-Cancelation.md new file mode 100644 index 00000000..f4d6cdf2 --- /dev/null +++ b/book/src/rfcs/RFC0007-Opinionated-Cancelation.md @@ -0,0 +1,63 @@ +# Description/title + +## Metadata + +* Author: nikomatsakis +* Date: 2021-05-15 +* Introduced in: N/A + +## Summary + +* Define panic as the one true way to handle cancellation in salsa queries +* Modify salsa queries to automatically panic when they are canceled +* Use a distinguished value for this panic so that people can test if the panic was a result of cancellation + +## Motivation + +Salsa's database model is fundamentally like a read-write lock. There is always a single *master copy* of the database which supports writes, and any number of concurrent *snapshots* that support reads. Whenever a write to the database occurs, any queries executing in those snapshots are considered *canceled*, because their results are based on stale data. The write blocks until they complete before it actually takes effect. It is therefore advantageous for those reads to complete as quickly as possible. + +Cancellation in salsa is currently quite minimal. Effectively, a flag becomes true, and queries can manually check for this flag. This is easy to forget to do. Moreover, we support two modes of cancellation: you can either use `Result` values or use `panic!`. In practice, though, there isn't much point to using `Result`: you can't really "recover" from cancellation. + +The largest user of salsa, rust-analyzer, uses a fairly opinionated and aggressive form of cancellation: + +* Every query is instrumented, using salsa's various hooks, to check for cancellation before it begins. +* If a query is canceled, then it immediately panics, using a special sentinel value. +* Any worker threads holding a snapshot of the DB recognize this value and go back to waiting for work. + +We propose to make this model of cancellation the *only* model of cancellation. + +## User's guide + +When you do a write to the salsa database, that write will block until any queries running in background threads have completed. You really want those queries to complete quickly, though, because they are now operating on stale data and their results are therefore not meaningful. To expedite the process, salsa will *cancel* those queries. That means that the queries will panic as soon as they try to execute another salsa query. Those panics occur using a sentinel value that you can check for if you wish. If you have a query that contains a long loop which does not execute any intermediate queries, salsa won't be able to cancel it automatically. You may wish to check for cancelation yourself by invoking the `panic_if_canceled` method. + +## Reference guide + +The changes required to implement this RFC are as follows: + +* Adjust comments on `is_current_revision_canceled`. +* Introduce a sentinel value `static SALSA_CANCELATION_TOKEN: SalsaCancelationToken = SalsaCancelationToken` that can be used with `panic_any!` +* Introduce a `panic_if_canceled` method into the runtime which checks whether cancelation has occured and panics if so. + * This should probably be inline for the `if` with an outlined function to do the actual panic. +* Modify the code for the various queries to invoke `panic_if_canceled` when they are invoked. + * QUESTION: Do we want to just check *every query access*, including cache hits? Probably. Is there a convenient time to do this in the runtime + +## Frequently asked questions + +### Isn't it hard to write panic-safe code? + +It is. However, the salsa runtime is panic-safe, and all salsa queries must already avoid side-effects for other reasons, so in our case, being panic-safe happens by default. + +### Isn't recovering from panics a bad idea? + +No. It's a bad idea to do "fine-grained" recovery from panics, but catching a panic at a high-level of your application and soldiering on is actually exactly how panics were meant to be used. This is especially true in salsa, since all code is already panic-safe. + +### Does this affect users of salsa who do not use threads? + +No. Cancelation in salsa only occurs when there are parallel readers and writers. + +### What about people using panic-as-abort? + +This does mean that salsa is not compatible with panic-as-abort. Strictly speaking, you could still use salsa in single-threaded mode, so that cancelation is not possible. + + + From 54e092b9848f13334478850d7143c8d194d21dc0 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Sat, 15 May 2021 06:07:02 -0400 Subject: [PATCH 2/5] fix title --- book/src/rfcs/RFC0007-Opinionated-Cancelation.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/book/src/rfcs/RFC0007-Opinionated-Cancelation.md b/book/src/rfcs/RFC0007-Opinionated-Cancelation.md index f4d6cdf2..21fbb825 100644 --- a/book/src/rfcs/RFC0007-Opinionated-Cancelation.md +++ b/book/src/rfcs/RFC0007-Opinionated-Cancelation.md @@ -1,4 +1,4 @@ -# Description/title +# Opinionated cancelation ## Metadata From be39f5d870922ea4fd80400c0ad838de6ef1e7cc Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Sat, 15 May 2021 09:26:28 -0400 Subject: [PATCH 3/5] Apply suggestions from code review Co-authored-by: Aleksey Kladov --- book/src/rfcs/RFC0007-Opinionated-Cancelation.md | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/book/src/rfcs/RFC0007-Opinionated-Cancelation.md b/book/src/rfcs/RFC0007-Opinionated-Cancelation.md index 21fbb825..d6806713 100644 --- a/book/src/rfcs/RFC0007-Opinionated-Cancelation.md +++ b/book/src/rfcs/RFC0007-Opinionated-Cancelation.md @@ -8,8 +8,8 @@ ## Summary -* Define panic as the one true way to handle cancellation in salsa queries -* Modify salsa queries to automatically panic when they are canceled +* Define stack unwinding as the one true way to handle cancellation in salsa queries +* Modify salsa queries to automatically initiate unwinding when they are canceled * Use a distinguished value for this panic so that people can test if the panic was a result of cancellation ## Motivation @@ -60,4 +60,3 @@ No. Cancelation in salsa only occurs when there are parallel readers and writers This does mean that salsa is not compatible with panic-as-abort. Strictly speaking, you could still use salsa in single-threaded mode, so that cancelation is not possible. - From 08cc09f7e39d0d42101efe0b4c6c110ffcf1ea26 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Sat, 15 May 2021 09:33:15 -0400 Subject: [PATCH 4/5] nits, sp --- .../rfcs/RFC0007-Opinionated-Cancelation.md | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/book/src/rfcs/RFC0007-Opinionated-Cancelation.md b/book/src/rfcs/RFC0007-Opinionated-Cancelation.md index d6806713..aa12aaa2 100644 --- a/book/src/rfcs/RFC0007-Opinionated-Cancelation.md +++ b/book/src/rfcs/RFC0007-Opinionated-Cancelation.md @@ -8,37 +8,37 @@ ## Summary -* Define stack unwinding as the one true way to handle cancellation in salsa queries +* Define stack unwinding as the one true way to handle cancelation in salsa queries * Modify salsa queries to automatically initiate unwinding when they are canceled -* Use a distinguished value for this panic so that people can test if the panic was a result of cancellation +* Use a distinguished value for this panic so that people can test if the panic was a result of cancelation ## Motivation Salsa's database model is fundamentally like a read-write lock. There is always a single *master copy* of the database which supports writes, and any number of concurrent *snapshots* that support reads. Whenever a write to the database occurs, any queries executing in those snapshots are considered *canceled*, because their results are based on stale data. The write blocks until they complete before it actually takes effect. It is therefore advantageous for those reads to complete as quickly as possible. -Cancellation in salsa is currently quite minimal. Effectively, a flag becomes true, and queries can manually check for this flag. This is easy to forget to do. Moreover, we support two modes of cancellation: you can either use `Result` values or use `panic!`. In practice, though, there isn't much point to using `Result`: you can't really "recover" from cancellation. +cancelation in salsa is currently quite minimal. Effectively, a flag becomes true, and queries can manually check for this flag. This is easy to forget to do. Moreover, we support two modes of cancelation: you can either use `Result` values or use unwinding. In practice, though, there isn't much point to using `Result`: you can't really "recover" from cancelation. -The largest user of salsa, rust-analyzer, uses a fairly opinionated and aggressive form of cancellation: +The largest user of salsa, rust-analyzer, uses a fairly opinionated and aggressive form of cancelation: -* Every query is instrumented, using salsa's various hooks, to check for cancellation before it begins. +* Every query is instrumented, using salsa's various hooks, to check for cancelation before it begins. * If a query is canceled, then it immediately panics, using a special sentinel value. * Any worker threads holding a snapshot of the DB recognize this value and go back to waiting for work. -We propose to make this model of cancellation the *only* model of cancellation. +We propose to make this model of cancelation the *only* model of cancelation. ## User's guide -When you do a write to the salsa database, that write will block until any queries running in background threads have completed. You really want those queries to complete quickly, though, because they are now operating on stale data and their results are therefore not meaningful. To expedite the process, salsa will *cancel* those queries. That means that the queries will panic as soon as they try to execute another salsa query. Those panics occur using a sentinel value that you can check for if you wish. If you have a query that contains a long loop which does not execute any intermediate queries, salsa won't be able to cancel it automatically. You may wish to check for cancelation yourself by invoking the `panic_if_canceled` method. +When you do a write to the salsa database, that write will block until any queries running in background threads have completed. You really want those queries to complete quickly, though, because they are now operating on stale data and their results are therefore not meaningful. To expedite the process, salsa will *cancel* those queries. That means that the queries will panic as soon as they try to execute another salsa query. Those panics occur using a sentinel value that you can check for if you wish. If you have a query that contains a long loop which does not execute any intermediate queries, salsa won't be able to cancel it automatically. You may wish to check for cancelation yourself by invoking the `unwind_if_canceled` method. ## Reference guide The changes required to implement this RFC are as follows: * Adjust comments on `is_current_revision_canceled`. -* Introduce a sentinel value `static SALSA_CANCELATION_TOKEN: SalsaCancelationToken = SalsaCancelationToken` that can be used with `panic_any!` -* Introduce a `panic_if_canceled` method into the runtime which checks whether cancelation has occured and panics if so. +* Introduce a sentinel type/value `SalsaCancelationToken` that can be used with [`resume_unwind`](https://doc.rust-lang.org/std/panic/fn.resume_unwind.html) +* Introduce a `unwind_if_canceled` method into the runtime which checks whether cancelation has occured and panics if so. * This should probably be inline for the `if` with an outlined function to do the actual panic. -* Modify the code for the various queries to invoke `panic_if_canceled` when they are invoked. +* Modify the code for the various queries to invoke `unwind_if_canceled` when they are invoked or validated. * QUESTION: Do we want to just check *every query access*, including cache hits? Probably. Is there a convenient time to do this in the runtime ## Frequently asked questions From 67207d8d0d58b53cff6e27270bdadf4907142d3f Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Thu, 27 May 2021 20:24:38 -0400 Subject: [PATCH 5/5] update with links to implementation --- book/src/rfcs/RFC0007-Opinionated-Cancelation.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/book/src/rfcs/RFC0007-Opinionated-Cancelation.md b/book/src/rfcs/RFC0007-Opinionated-Cancelation.md index aa12aaa2..0eedfb82 100644 --- a/book/src/rfcs/RFC0007-Opinionated-Cancelation.md +++ b/book/src/rfcs/RFC0007-Opinionated-Cancelation.md @@ -4,7 +4,7 @@ * Author: nikomatsakis * Date: 2021-05-15 -* Introduced in: N/A +* Introduced in: [salsa-rs/salsa#265](https://github.com/salsa-rs/salsa/pull/265) ## Summary @@ -34,12 +34,12 @@ When you do a write to the salsa database, that write will block until any queri The changes required to implement this RFC are as follows: -* Adjust comments on `is_current_revision_canceled`. -* Introduce a sentinel type/value `SalsaCancelationToken` that can be used with [`resume_unwind`](https://doc.rust-lang.org/std/panic/fn.resume_unwind.html) -* Introduce a `unwind_if_canceled` method into the runtime which checks whether cancelation has occured and panics if so. +* Remove on `is_current_revision_canceled`. +* Introduce a sentinel cancellation token that can be used with [`resume_unwind`](https://doc.rust-lang.org/std/panic/fn.resume_unwind.html) +* Introduce a `unwind_if_canceled` method into the `Database` which checks whether cancelation has occured and panics if so. + * This method also triggers a `salsa_event` callback. * This should probably be inline for the `if` with an outlined function to do the actual panic. * Modify the code for the various queries to invoke `unwind_if_canceled` when they are invoked or validated. - * QUESTION: Do we want to just check *every query access*, including cache hits? Probably. Is there a convenient time to do this in the runtime ## Frequently asked questions