fix: guard recursive array materialization#1040
Open
He-Pin wants to merge 1 commit into
Open
Conversation
Motivation: Nested lazy arrays can re-enter the same array value during manifestation. The generic materializer and ByteRenderer previously only guarded object cycles, so recursive arrays could grow partial output until stack or memory failure. Modification: Track currently materialized arrays and objects in the existing identity guard, with try/finally cleanup on recursive, stackless, and ByteRenderer direct paths. Keep compact RangeArr and ByteArr fast paths outside the guard. Add golden regressions for nested lazy array cycles and shared acyclic arrays. Result: Recursive array manifestation now fails before writing partial JSON while shared non-cyclic arrays still materialize normally.
d9831b7 to
9909a95
Compare
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Motivation
The previous materialization guard only tracked
Val.Obj. A nested lazy array can re-enter the sameVal.Arrduring manifestation, especially through the fusedByteRendererpath, and sjsonnet would emit partial JSON until stack/memory failure. Recursive array manifestation should fail before any output grows, matching C++ jsonnet, go-jsonnet, and jrsonnet behavior.Reproducer:
RUNTIME ERROR: max stack frames exceeded.RUNTIME ERROR: max stack frames exceeded.stack overflowsjsonnet.Error: Stackoverflow while materializingModification
visitedObjects: IdentityHashMap[Val.Obj, Boolean]withvisitedContainers: IdentityHashMap[Val, Boolean].Materializer, stackless frame push/pop paths, and directByteRenderermaterialization.RangeArrandByteArrfast paths outside the guard — they are compact numeric leaf arrays, avoiding extra map traffic on hot paths.subvisitor outside the while loop inByteArrstackless fast path to avoid repeatedsubVisitoraccess andasInstanceOfcast per element.try/finallycleanup on all three materialization paths to avoid retaining containers after errors.Result
array_copy_views.jsonnet.RangeArr/ByteArrfast paths.References
core/vm.cppchecks max stack frames innewCalltest_suite/error.array_recursive_manifest.jsonnet