Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bug: Mutex locks not released on error #4557

Merged
merged 3 commits into from
Jun 12, 2018
Merged

Bug: Mutex locks not released on error #4557

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8f93e34
nonce-tracker - wrap nonce calculations in try-catch and release lock…
kumavis Jun 12, 2018
177cc3f
metamask - ensure all nonce locks are released
kumavis Jun 12, 2018
604289c
controllers - transaction - prefer log over console
kumavis Jun 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions app/scripts/controllers/transactions/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ class TransactionController extends EventEmitter {
// add default tx params
txMeta = await this.addTxGasDefaults(txMeta)
} catch (error) {
console.log(error)
log.warn(error)
this.txStateManager.setTxStatusFailed(txMeta.id, error)
throw error
}
Expand Down Expand Up @@ -264,7 +264,12 @@ class TransactionController extends EventEmitter {
// must set transaction to submitted/failed before releasing lock
nonceLock.releaseLock()
} catch (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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@kumavis kumavis Jun 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i treated it as a black box and assumed errors could come from it
its possible errors could come from an eventemitter handler

} catch (err) {
log.error(err)
}
// must set transaction to submitted/failed before releasing lock
if (nonceLock) nonceLock.releaseLock()
// continue with error chain
Expand Down
54 changes: 30 additions & 24 deletions app/scripts/controllers/transactions/nonce-tracker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,29 +49,35 @@ class NonceTracker {
await this._globalMutexFree()
// await lock free, then take lock
const releaseLock = await this._takeMutex(address)
// evaluate multiple nextNonce strategies
const nonceDetails = {}
const networkNonceResult = await this._getNetworkNextNonce(address)
const highestLocallyConfirmed = this._getHighestLocallyConfirmed(address)
const nextNetworkNonce = networkNonceResult.nonce
const highestSuggested = Math.max(nextNetworkNonce, highestLocallyConfirmed)

const pendingTxs = this.getPendingTransactions(address)
const localNonceResult = this._getHighestContinuousFrom(pendingTxs, highestSuggested) || 0

nonceDetails.params = {
highestLocallyConfirmed,
highestSuggested,
nextNetworkNonce,
try {
// evaluate multiple nextNonce strategies
const nonceDetails = {}
const networkNonceResult = await this._getNetworkNextNonce(address)
const highestLocallyConfirmed = this._getHighestLocallyConfirmed(address)
const nextNetworkNonce = networkNonceResult.nonce
const highestSuggested = Math.max(nextNetworkNonce, highestLocallyConfirmed)

const pendingTxs = this.getPendingTransactions(address)
const localNonceResult = this._getHighestContinuousFrom(pendingTxs, highestSuggested) || 0

nonceDetails.params = {
highestLocallyConfirmed,
highestSuggested,
nextNetworkNonce,
}
nonceDetails.local = localNonceResult
nonceDetails.network = networkNonceResult

const nextNonce = Math.max(networkNonceResult.nonce, localNonceResult.nonce)
assert(Number.isInteger(nextNonce), `nonce-tracker - nextNonce is not an integer - got: (${typeof nextNonce}) "${nextNonce}"`)

// return nonce and release cb
return { nextNonce, nonceDetails, releaseLock }
} catch (err) {
// release lock if we encounter an error
releaseLock()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a great candidate for the approved bug, I'm so excited to see you find this.

throw err
}
nonceDetails.local = localNonceResult
nonceDetails.network = networkNonceResult

const nextNonce = Math.max(networkNonceResult.nonce, localNonceResult.nonce)
assert(Number.isInteger(nextNonce), `nonce-tracker - nextNonce is not an integer - got: (${typeof nextNonce}) "${nextNonce}"`)

// return nonce and release cb
return { nextNonce, nonceDetails, releaseLock }
}

async _getCurrentBlock () {
Expand All @@ -85,8 +91,8 @@ class NonceTracker {

async _globalMutexFree () {
const globalMutex = this._lookupMutex('global')
const release = await globalMutex.acquire()
release()
const releaseLock = await globalMutex.acquire()
releaseLock()
}

async _takeMutex (lockId) {
Expand Down
4 changes: 2 additions & 2 deletions app/scripts/controllers/transactions/pending-tx-tracker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,14 +196,14 @@ class PendingTransactionTracker extends EventEmitter {
async _checkPendingTxs () {
const signedTxList = this.getPendingTransactions()
// in order to keep the nonceTracker accurate we block it while updating pending transactions
const nonceGlobalLock = await this.nonceTracker.getGlobalLock()
const { releaseLock } = await this.nonceTracker.getGlobalLock()
try {
await Promise.all(signedTxList.map((txMeta) => this._checkPendingTx(txMeta)))
} catch (err) {
log.error('PendingTransactionWatcher - Error updating pending transactions')
log.error(err)
}
nonceGlobalLock.releaseLock()
releaseLock()
}

/**
Expand Down
20 changes: 8 additions & 12 deletions app/scripts/metamask-controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,28 +436,24 @@ module.exports = class MetamaskController extends EventEmitter {
* @returns {Object} vault
*/
async createNewVaultAndKeychain (password) {
const release = await this.createVaultMutex.acquire()
let vault

const releaseLock = await this.createVaultMutex.acquire()
Copy link
Contributor

@bitpshr bitpshr Jun 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the mutex acquisition ever fail? Should we move this asynchronous operation inside the try / catch as well, or is that overly-cautious?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

acquisition should never fail

try {
let vault
const accounts = await this.keyringController.getAccounts()

if (accounts.length > 0) {
vault = await this.keyringController.fullUpdate()

} else {
vault = await this.keyringController.createNewVaultAndKeychain(password)
const accounts = await this.keyringController.getAccounts()
this.preferencesController.setAddresses(accounts)
this.selectFirstIdentity()
}
release()
releaseLock()
return vault
} catch (err) {
release()
releaseLock()
throw err
}

return vault
}

/**
Expand All @@ -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()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very readable renaming.

try {
// clear known identities
this.preferencesController.setAddresses([])
Expand All @@ -476,10 +472,10 @@ module.exports = class MetamaskController extends EventEmitter {
const accounts = await this.keyringController.getAccounts()
this.preferencesController.setAddresses(accounts)
this.selectFirstIdentity()
release()
releaseLock()
return vault
} catch (err) {
release()
releaseLock()
throw err
}
}
Expand Down