Bug: Mutex locks not released on error #4557
Conversation
|
@frankiebee you were working on this sort of thing before, so you might be interested in these changes |
| try { | ||
| this.txStateManager.setTxStatusFailed(txId, err) | ||
| } catch (err) { | ||
| console.error(err) |
| this.txStateManager.setTxStatusFailed(txId, err) | ||
| // this is try-catch wrapped so that we can guarantee that the nonceLock is released | ||
| try { | ||
| this.txStateManager.setTxStatusFailed(txId, err) |
There was a problem hiding this comment.
Was setTxStatusFailed() throwing uncaught errors? Looking at it, it seems so straight forward. In fact, even its own internal _setTxStatus(id, status) method has its own try/catch.
There was a problem hiding this comment.
i treated it as a black box and assumed errors could come from it
its possible errors could come from an eventemitter handler
| return { nextNonce, nonceDetails, releaseLock } | ||
| } catch (err) { | ||
| // release lock if we encounter an error | ||
| releaseLock() |
There was a problem hiding this comment.
This looks like a great candidate for the approved bug, I'm so excited to see you find this.
| @@ -466,7 +462,7 @@ module.exports = class MetamaskController extends EventEmitter { | |||
| * @param {} seed | |||
| */ | |||
| async createNewVaultAndRestore (password, seed) { | |||
| const release = await this.createVaultMutex.acquire() | |||
| const releaseLock = await this.createVaultMutex.acquire() | |||
|
theres some style changes for consistency, the biggest change is the addition of 2 try-catchs where we were doing things that could error |
| const release = await this.createVaultMutex.acquire() | ||
| let vault | ||
|
|
||
| const releaseLock = await this.createVaultMutex.acquire() |
There was a problem hiding this comment.
Can the mutex acquisition ever fail? Should we move this asynchronous operation inside the try / catch as well, or is that overly-cautious?
There was a problem hiding this comment.
acquisition should never fail
Fixes #4343
Auditing and fixing use of locks: