Skip to content
This repository has been archived by the owner on Jan 19, 2021. It is now read-only.

get / pop of undefined #33

Closed
wants to merge 2 commits into from
Closed

get / pop of undefined #33

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f152926
Fixed race conditions around `_getDBs`
clehene Jan 31, 2018
75bc8da
Fixed incorrect suppression of `db.get` `err` (mainly 'not found')
clehene Jan 31, 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
115 changes: 70 additions & 45 deletions baseTrie.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,15 +65,16 @@ function Trie (db, root) {
Trie.prototype.get = function (key, cb) {
var self = this

key = ethUtil.toBuffer(key)

self.findPath(key, function (err, node, remainder, stack) {
var value = null
if (node && remainder.length === 0) {
value = node.value
}

cb(err, value)
cb = callTogether(cb, self.sem.leave)
self.sem.take(function () {
key = ethUtil.toBuffer(key)
self.findPath(key, function (err, node, remainder, stack) {
var value = null
if (node && remainder.length === 0) {
value = node.value
}
cb(err, value)
})
})
}

Expand Down Expand Up @@ -142,9 +143,23 @@ Trie.prototype.del = function (key, cb) {
* Retrieves a raw value in the underlying db
* @method getRaw
* @param {Buffer} key
* @param {Function} callback A callback `Function`, which is given the arguments `err` - for errors that may have occured and `value` - the found value in a `Buffer` or if no value was found `null`.
* @param {Function} cb A callback `Function`, which is given the arguments `err` - for errors that may have occured and `value` - the found value in a `Buffer` or if no value was found `null`.
* @param {Boolean} locked flag to
*/
Trie.prototype.getRaw = function (key, cb) {

Trie.prototype.getRaw = function (key, cb, locked) {
var self = this
if (!locked) {
cb = callTogether(cb, this.sem.leave)
self.sem.take(function () {
self._getRaw(key, cb)
})
}
else {
self._getRaw(key, cb)
}
}
Trie.prototype._getRaw = function (key, cb) {
key = ethUtil.toBuffer(key)

function dbGet (db, cb2) {
Expand All @@ -153,12 +168,15 @@ Trie.prototype.getRaw = function (key, cb) {
valueEncoding: 'binary'
}, function (err, foundNode) {
if (err || !foundNode) {
// not passing err as it makes the wrapping async iteration to fail and
// interferes with asyncFirstSeries logic, causing an early return
cb2(null, null)
} else {
cb2(null, foundNode)
}
})
}
// TODO replace with async.race / async.tryEach
asyncFirstSeries(this._getDBs, dbGet, cb)
}

Expand All @@ -169,15 +187,14 @@ Trie.prototype._lookupNode = function (node, cb) {
} else {
this.getRaw(node, function (err, value) {
if (err) {
throw err
}

if (value) {
cb(err)
} else if (value) {
value = new TrieNode(rlp.decode(value))
cb(null, value)
} else {
cb()
}

cb(value)
})
}, true)
}
}

Expand Down Expand Up @@ -311,24 +328,27 @@ Trie.prototype._findNode = function (key, root, stack, cb) {
* Finds all nodes that store k,v values
*/
Trie.prototype._findValueNodes = function (onFound, cb) {
this._walkTrie(this.root, function (nodeRef, node, key, walkController) {
var fullKey = key
cb = callTogether(cb, this.sem.leave)
this.sem.take(function () {
this._walkTrie(this.root, function (nodeRef, node, key, walkController) {
var fullKey = key

if (node.key) {
fullKey = key.concat(node.key)
}
if (node.key) {
fullKey = key.concat(node.key)
}

if (node.type === 'leaf') {
// found leaf node!
onFound(nodeRef, node, fullKey, walkController.next)
} else if (node.type === 'branch' && node.value) {
// found branch with value
onFound(nodeRef, node, fullKey, walkController.next)
} else {
// keep looking for value nodes
walkController.next()
}
}, cb)
if (node.type === 'leaf') {
// found leaf node!
onFound(nodeRef, node, fullKey, walkController.next)
} else if (node.type === 'branch' && node.value) {
// found branch with value
onFound(nodeRef, node, fullKey, walkController.next)
} else {
// keep looking for value nodes
walkController.next()
}
}, cb)
})
}

/*
Expand Down Expand Up @@ -452,17 +472,19 @@ Trie.prototype._walkTrie = function (root, onNode, onDone) {
return onDone()
}

self._lookupNode(root, function (node) {
processNode(root, node, null, function (err) {
self._lookupNode(root, function (err, node) {
walkNode(err, root, node, null, function (err) {
if (err) {
return onDone(err)
}
onDone.apply(null, returnValues)
})
})

function processNode (nodeRef, node, key, cb) {
if (!node) return cb()
function walkNode (err, nodeRef, node, key, cb) {
if (!node) {
return cb(err)
}
if (aborted) return cb()
var stopped = false
key = key || []
Expand Down Expand Up @@ -490,17 +512,17 @@ Trie.prototype._walkTrie = function (root, onNode, onDone) {
var keyExtension = childData[0]
var childRef = childData[1]
var childKey = key.concat(keyExtension)
self._lookupNode(childRef, function (childNode) {
processNode(childRef, childNode, childKey, cb)
self._lookupNode(childRef, function (err, childNode) {
walkNode(null, childRef, childNode, childKey, cb)
})
}, cb)
},
only: function (childIndex) {
var childRef = node.getValue(childIndex)
self._lookupNode(childRef, function (childNode) {
self._lookupNode(childRef, function (err, childNode) {
var childKey = key.slice()
childKey.push(childIndex)
processNode(childRef, childNode, childKey, cb)
walkNode(null, childRef, childNode, childKey, cb)
})
}
}
Expand Down Expand Up @@ -642,7 +664,7 @@ Trie.prototype._deleteNode = function (key, stack, cb) {
var branchNodeKey = branchNodes[0][0]

// look up node
this._lookupNode(branchNode, function (foundNode) {
this._lookupNode(branchNode, function (err, foundNode) {
key = processBranchNode(key, branchNodeKey, foundNode, parentNode, stack, opStack)
self._saveStack(key, stack, opStack, cb)
})
Expand Down Expand Up @@ -746,7 +768,10 @@ Trie.prototype.batch = function (ops, cb) {
*/
Trie.prototype.checkRoot = function (root, cb) {
root = ethUtil.toBuffer(root)
this._lookupNode(root, function (value) {
cb(null, !!value)
cb = callTogether(cb, self.sem.leave)
self.sem.take(function () {
this._lookupNode(root, function (err, value) {
cb(null, !!value)
})
})
}
34 changes: 21 additions & 13 deletions checkpoint-interface.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,15 @@ function checkpointInterface (trie) {
*/
function checkpoint () {
var self = this
var wasCheckpoint = self.isCheckpoint
self._checkpoints.push(self.root)
if (!wasCheckpoint && self.isCheckpoint) {
self._enterCpMode()
}
self.sem.take(function () {
Copy link

@benjamincburns benjamincburns Jan 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

checkpoint used to be synchronous, but this change makes it asynchronous. As a result, this function should accept a callback argument.

This change will likely have a ripple effect into ethereumjs-vm and ganache-core, if not other places.

Copy link
Author

Choose a reason for hiding this comment

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

You're right. I had it on the list but then missed it. Unfortunately this fix is not legit without the callback as it defers the commit and it also unlikely to work without getting the semaphore there as enterCpMode mutates the db arrays for get and puts.
I'll look at it this evening and check where it gets called. I'll close this for now

var wasCheckpoint = self.isCheckpoint
self._checkpoints.push(self.root)
if (!wasCheckpoint && self.isCheckpoint) {
self._enterCpMode()
}
self.sem.leave()
})

}

/**
Expand Down Expand Up @@ -171,15 +175,19 @@ ScratchReadStream.prototype._read = function () {
var self = this
if (!self._started) {
self._started = true
self.trie._findDbNodes(function (nodeRef, node, key, next) {
self.push({
key: nodeRef,
value: node.serialize()
self.trie.sem.take(function () {
self.trie._findDbNodes(function (nodeRef, node, key, next) {
self.push({
key: nodeRef,
value: node.serialize()
})
next()
}, function () {
// close stream
self.push(null)
self.trie.sem.leave()
})
next()
}, function () {
// close stream
self.push(null)
})

}
}
15 changes: 9 additions & 6 deletions util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,21 +58,24 @@ function callTogether () {
/**
* Take a collection of async fns, call the cb on the first to return a truthy value.
* If all run without a truthy result, return undefined
*
* TODO replace with async race / tryEach
* intended behavior depends on swallowing any error
*/
function asyncFirstSeries (array, iterator, cb) {
function asyncFirstSeries (array, processItem, cb) {
var didComplete = false
async.eachSeries(array, function (item, next) {
if (didComplete) return next
iterator(item, function (err, result) {
if (result) {
processItem(item, function (err, returnVal) {
if (returnVal) {
didComplete = true
process.nextTick(cb.bind(null, null, result))
process.nextTick(cb.bind(null, null, returnVal))
}
next(err)
})
}, function () {
}, function (err) {
if (!didComplete) {
cb()
cb({'err': 'no result'})
}
})
}