sp-api: Support nested transactions #14447
Conversation
Adds support for nested transactions in `sp-api` by using `execute_in_transaction`. This was working until a recent refactor, but this was actually not intended. However, supporting nested transactions is a worthwhile feature to have. So, this pr "brings it back" and adds a test to ensure it will not break.
bkchr
added
A0-please_review
bkchr
added
the
D3-trivial 🧸
| *std::cell::RefCell::borrow_mut(&self.commit_on_success) = false; | ||
| let old_value = std::cell::RefCell::replace(&self.in_transaction, true); | ||
| let res = call(self); | ||
| *std::cell::RefCell::borrow_mut(&self.commit_on_success) = true; | ||
| *std::cell::RefCell::borrow_mut(&self.in_transaction) = old_value; |
There was a problem hiding this comment.
This might not matter in practice (?), but commit_or_rollback_transaction can panic, and now we're going to support nested transactions, which means that call can panic, which means that if it does then the in_transaction won't get restored to the old value.
There was a problem hiding this comment.
Yeah I know, but the type should not be unwind safe (which is the case). However, I added a test to ensure this.
There was a problem hiding this comment.
Yeah I know, but the type should not be unwind safe (which is the case). However, I added a test to ensure this.
Hmmm... to be honest, I'm not sure how useful that is considering how borderline useless the UnwindSafe trait is. (Since e.g. even &mut T ends up being not UnwindSafe, which makes it extremely false positive-y, which makes the use of AssertUnwindSafe very common.)
Maybe instead of in_transaction: bool we could have e.g. transaction_depth: usize and increment/decrement it when we enter/leave a transaction? I think that'd be a little nicer. (And in case there's any funny business going to it'd be easier to debug since you'd have a stale non-zero counter.)
But it's up to you.
| sp_io::storage::set(&key, &value); | ||
|
|
||
| if panic { | ||
| panic!("I'm just following my master"); |
| *std::cell::RefCell::borrow_mut(&self.commit_on_success) = false; | ||
| let old_value = std::cell::RefCell::replace(&self.in_transaction, true); | ||
| let res = call(self); | ||
| *std::cell::RefCell::borrow_mut(&self.commit_on_success) = true; | ||
| *std::cell::RefCell::borrow_mut(&self.in_transaction) = old_value; |
There was a problem hiding this comment.
Yeah I know, but the type should not be unwind safe (which is the case). However, I added a test to ensure this.
Hmmm... to be honest, I'm not sure how useful that is considering how borderline useless the UnwindSafe trait is. (Since e.g. even &mut T ends up being not UnwindSafe, which makes it extremely false positive-y, which makes the use of AssertUnwindSafe very common.)
Maybe instead of in_transaction: bool we could have e.g. transaction_depth: usize and increment/decrement it when we enter/leave a transaction? I think that'd be a little nicer. (And in case there's any funny business going to it'd be easier to debug since you'd have a stale non-zero counter.)
But it's up to you.
| @@ -229,7 +229,7 @@ fn generate_runtime_api_base_structures() -> Result<TokenStream> { | |||
| #crate_::std_enabled! { | |||
| pub struct RuntimeApiImpl<Block: #crate_::BlockT, C: #crate_::CallApiAt<Block> + 'static> { | |||
| call: &'static C, | |||
| commit_on_success: std::cell::RefCell<bool>, | |||
| in_transaction: std::cell::RefCell<bool>, | |||
There was a problem hiding this comment.
Hm, can't this use Cell instead of RefCell?
|
bot merge |
|
Waiting for commit status. |
|
Merge cancelled due to error. Error: Statuses failed for 3b87f5b |
* sp-api: Support nested transactions Adds support for nested transactions in `sp-api` by using `execute_in_transaction`. This was working until a recent refactor, but this was actually not intended. However, supporting nested transactions is a worthwhile feature to have. So, this pr "brings it back" and adds a test to ensure it will not break. * Make clippy happy * Assert that the runtime api type is not unwind safe * Count number of transactions
Adds support for nested transactions in
sp-apiby usingexecute_in_transaction. This was working until a recent refactor, but this was actually not intended. However, supporting nested transactions is a worthwhile feature to have. So, this pr "brings it back" and adds a test to ensure it will not break.