Add mutex to getSnapState to prevent concurrent decryption
#3234
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3234 +/- ##
=======================================
Coverage 94.98% 94.98%
=======================================
Files 511 511
Lines 11300 11301 +1
Branches 1737 1737
=======================================
+ Hits 10733 10734 +1
Misses 567 567 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Mrtenz
force-pushed
the
mrtenz/get-state-mutex
branch
from
40f1110 to
6c486b3
Compare
| async getSnapState(snapId: SnapId, encrypted: boolean): Promise<Json> { | ||
| const runtime = this.#getRuntimeExpect(snapId); | ||
| const cachedState = encrypted ? runtime.state : runtime.unencryptedState; | ||
| return await runtime.getStateMutex.runExclusive(async () => { |
There was a problem hiding this comment.
With this cached requests would still have to queue to access the cache, right? That seems less than ideal 🤔
There was a problem hiding this comment.
Would it work if we have a cacheMutex that nests this one?
There was a problem hiding this comment.
What do you mean by nesting in this case?
With this cached requests would still have to queue to access the cache, right? That seems less than ideal 🤔
Yeah, but cached access should be much quicker, so I don't think it's a big issue?
There was a problem hiding this comment.
Let's try this as-is and we can always move some logic outside of the mutex block
| /** | ||
| * A mutex to prevent concurrent state decryption. | ||
| */ | ||
| getStateMutex: Mutex; |
There was a problem hiding this comment.
Wondering if we could re-use the stateMutex
There was a problem hiding this comment.
It could, but I'm not sure if it should? We can do encryption and decryption at the same time, they don't depend on each other.
Mrtenz
force-pushed
the
mrtenz/get-state-mutex
branch
from
6c486b3 to
64549e3
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
This wraps
getSnapStatein a mutex, to ensure we only decrypt once when calling it at the same time. The first call will decrypt the state and cache it in the Snaps runtime, and the next call will just use the cached result.