This repository has been archived by the owner on Jan 19, 2021. It is now read-only.
get / pop of undefined
#33
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This includes all high level operations that either mutate `_getDBs` (e.g. `checkpoint`) or access it (mainly anything that eventually calls `getRaw`) Reference trufflesuite/ganache#453
which, unde certain conditions cause "Cannot read property 'pop' of undefined" Ref: * trufflesuite/ganache#417 * trufflesuite/ganache#453
| if (!wasCheckpoint && self.isCheckpoint) { | ||
| self._enterCpMode() | ||
| } | ||
| self.sem.take(function () { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes conditions described in trufflesuite/ganache#453 and trufflesuite/ganache#417
getonundefinedis caused by races aroundgetDBsWhen doing a checkpoint we end up with two dbs and all reads (getRaw->db.get) happen on both asynchronously. When things are committed or reverted the second db goes away and the in-flight get will try the second instance (think
db[0] / db[1]) that's not there anymore and fail with undefined .One case (in
getRaw) was trickier and is partially a break of encapsulation problem:getRawshould probably be private but is called directly from Account.This means that access needs to be synchronized around anything that would race for db instances.
Many of the operations were already using semaphores for this however some didn't (e.g.
checkpoint,get). The problem with the last race was due to direct access togetRawfromAccount.prototype.getCode. Again IMO this is mostly an encapsulation problem as get should probably be used instead (and perhaps that's the right fix, although I'd say it would also require makinggetRaw"private".The alternative (which is what is include in the patch) for a local fix is to mark the under-lock state with a flag (param), based on which's absence (if flag is off) the semaphore can be acquired to avoid the race.
The pop issue, is a logical one and happened due to propagation (or lack thereof) of
not founderrors fromdb.getoperations.Lastly, the merkle-patricia-tree code is more complex than it perhaps should given the functionality (deep callback stack, mutual recursion, heavy async, etc.). In addition the stuff around it, there may be other issues lying around,
As a caveat - now with the get / pop issues out of the way I can sometimes see
Initially I was concerned that they were somehow introduced by these fixes and tried to see how but that wasn't the case (not sure how they occur though) however I've confirmed them without the fixes as well.