Fixed shared cache race on import #4194
Conversation
There was a problem hiding this comment.
LGTM. Probably this comment must be updated as well, because Backend actually acquires the lock itself starting from this PR. Also maybe worth mentioning that no states should be destroyed when import lock is held (this seems a bit fragile)?
arkpar
added
A3-in_progress
Good point. It would be better to lock it outside. |
arkpar
added
A0-please_review
client/src/call_executor.rs
Outdated
| @@ -96,7 +96,10 @@ where | |||
| None, | |||
| ) | |||
| .map(|(result, _, _)| result)?; | |||
| self.backend.destroy_state(state)?; | |||
| { | |||
| let _ = self.backend.get_import_lock().read(); | |||
There was a problem hiding this comment.
That is not correct. It drops immediately the lock!
It is also not required to create a sub context here, as the functions end after it anyway.
There was a problem hiding this comment.
That is not correct. It drops immediately the lock!
How so? The RwLockReadGuard returned by read lives till the end of the block.
It is also not required to create a sub context here, as the functions end after it anyway.
Here it actually makes sense because we don't want to hold the lock for the duration of into_encoded
is_bestfunctor was checked under cache lock, which was supposed to guarantee consistency. But in reality it does not, because cache is also updated when validating transactions outside of import lock. So this is removed.Rwlockinstead ofMutexfor extra performance.