In some cases, the value of Uverse
is incomplete [WIP]
#2067
Labels
🐞 bug
Something isn't working
Milestone
Uverse
is incomplete [WIP]
#2067
WIP is the bug report, as we haven't fully finished investigating; but I'm opening this bug report to get some eyes on this...
Symptoms of the bug
In pull request #2040, @ajnavarro disabled the GnoVM benchmarks, as one of them was causing a panic:
https://github.com/gnolang/gno/pull/2040/files#diff-eff7bc5631d991a2e20f1a1ebbd6b16d7ccdcffe4a6284b817a99aafb5daed3c
However, what's curious about this panic is that it happens only when you run all the benchmarks... not just the single
BenchmarkBenchdata
set.This is the panic message:
When was it introduced?
We started digging... Luckily, the original commit where the benchmarks were introduced did not show this panic; which meant that it was time for
git bisect
to shine; which pointed to this commit: 52485ceNow, obviously this is a bit weird... I tried changing up the uverse initialization, first of all to instead of using a simple singleton pattern:
To using
sync.Once
to guard the creation of uverseNode/uverseValue; this way we could make sure that once the functions return the values are fully "finalized".However this resulted in a deadlock; because, as crazy as it may be, Uverse generation depends on itself.
The fault is not entirely on
isUverseName
; its usages to avoid using reserved identifiers can be overcome by making sure, before calling it, thatpackageOf(last).PkgPath != uversePkgPath
. However, this doesn't address the ultimate problem: that uverse generation depends on itself; there are other dependency chains which eventually makeUverse()
generation result in a deadlock.The changes showing this error can be found on this branch
How can we proceed?
I think it's important that
Uverse
doesn't depend on itself when initialising; butDefineNative
(which it extensively uses) usesPreprocess
to get type information, which usesevalStaticTypeOf
, which initializes a throwawayMachine
, which when setting up the store wants to pre-setUverse()
in its cached packages.What we want:
Uverse
andUverseNode
should always be the same, regardless of where they are first calledsync.Once
is preferable.How does it tie back to the original panic message?
Working theory:
Bad workarounds which cannot be considered solutions
... but may still be useful to show the problem.
Substituting the code of Uverse with the following:
Resolves the problem, however it is terribly inefficient.
Doing this at the beginning of BenchmarkBenchdata:
... also solves the problem, as uverseValue will have to be re-initialized.
The text was updated successfully, but these errors were encountered: