diff --git a/jsonpull/jsonpull.cpp b/jsonpull/jsonpull.cpp index c96adcc0..6e8a534d 100644 --- a/jsonpull/jsonpull.cpp +++ b/jsonpull/jsonpull.cpp @@ -675,7 +675,81 @@ json_object_ptr json_read_tree(json_pull_ptr p) { return nullptr; } +// Splice `o` out of its parent's array or object, dropping the +// parent's owning reference. After this returns, the parent no longer +// holds any pointer to `o`; the caller's reference is the only thing +// keeping the subtree alive. +// +// For a hash, removing a single key or value individually would +// disturb the surrounding key/value pairing, so we replace the removed +// half with a placeholder JSON_NULL and only erase the entry once both +// halves have been detached. This matches the historical +// json_disconnect semantics. +static void splice_from_parent(json_object *o) { + if (o == nullptr) { + return; + } + + json_object *parent = o->parent; + if (parent == nullptr) { + return; + } + + if (parent->type == JSON_ARRAY) { + auto &arr = parent->array(); + for (size_t i = 0; i < arr.size(); i++) { + if (arr[i].get() == o) { + arr.erase(arr.begin() + i); + break; + } + } + } else if (parent->type == JSON_HASH) { + auto &entries = parent->entries(); + for (size_t i = 0; i < entries.size(); i++) { + auto &e = entries[i]; + if (e.key.get() == o) { + e.key = fabricate_object(parent->parser, parent, JSON_NULL); + if (e.value != nullptr && e.value->type == JSON_NULL && e.key->type == JSON_NULL) { + entries.erase(entries.begin() + i); + } + break; + } + if (e.value.get() == o) { + e.value = fabricate_object(parent->parser, parent, JSON_NULL); + if (e.key != nullptr && e.key->type == JSON_NULL && e.value->type == JSON_NULL) { + entries.erase(entries.begin() + i); + } + break; + } + } + } +} + +// json_free splices `o` out of its parent (if any) so that the +// parent no longer keeps the subtree alive, then drops the caller's +// reference. The subtree is freed when the last reference is gone +// (typically right here, since the parent's reference was just +// dropped). geojson-loop.cpp relies on this to release each feature +// after it has been serialized, so that already-serialized features +// don't sit in memory while subsequent features are parsed. +// +// If `o` is the parser's current root (the most recently completed +// top-level value), drop the parser's reference too -- otherwise a +// line-delimited stream would always hold the previously-completed +// feature until the next one started parsing. +// +// Unlike json_disconnect, this does NOT walk the subtree clearing +// parent/parser back-pointers, because the subtree is about to be +// destroyed and those pointers will never be observed again. void json_free(json_object_ptr &o) { + if (o != nullptr) { + splice_from_parent(o.get()); + + json_pull *parser = o->parser; + if (parser != nullptr && parser->root.get() == o.get()) { + parser->root.reset(); + } + } o.reset(); } @@ -710,44 +784,7 @@ void json_disconnect(json_object_ptr o) { // Splice o out of its parent's array or object. The parent's vector // holds the shared_ptr to this child; erasing it removes one reference, // but the caller still holds `o`, so the subtree stays alive. - - json_object *parent = o->parent; - if (parent != nullptr) { - if (parent->type == JSON_ARRAY) { - auto &arr = parent->array(); - for (size_t i = 0; i < arr.size(); i++) { - if (arr[i].get() == o.get()) { - arr.erase(arr.begin() + i); - break; - } - } - } else if (parent->type == JSON_HASH) { - auto &entries = parent->entries(); - - for (size_t i = 0; i < entries.size(); i++) { - auto &e = entries[i]; - if (e.key.get() == o.get()) { - // Leave a NULL placeholder in the key slot so the - // surrounding value isn't shifted; if the corresponding - // value is also detached the pair is removed below. - e.key = fabricate_object(parent->parser, parent, JSON_NULL); - - if (e.value != nullptr && e.value->type == JSON_NULL && e.key->type == JSON_NULL) { - entries.erase(entries.begin() + i); - } - break; - } - if (e.value.get() == o.get()) { - e.value = fabricate_object(parent->parser, parent, JSON_NULL); - - if (e.key != nullptr && e.key->type == JSON_NULL && e.value->type == JSON_NULL) { - entries.erase(entries.begin() + i); - } - break; - } - } - } - } + splice_from_parent(o.get()); // Drop the parser's reference to this subtree if it was the root. json_pull *parser = o->parser; diff --git a/unit.cpp b/unit.cpp index 0db13ee9..bbd1a6d2 100644 --- a/unit.cpp +++ b/unit.cpp @@ -162,3 +162,74 @@ TEST_CASE("jsonpull surrogate-pair regression", "[jsonpull][surrogate]") { const std::string buggy = "\xF0\x9F\x90\x80"; REQUIRE(o->string() != buggy); } + +// geojson-loop.cpp calls json_free(j) after jfa->add_feature has +// serialized the feature, intending to drop the JSON subtree from the +// in-progress parse tree so that already-serialized features don't sit +// in memory while subsequent features are parsed. That intent was +// never tested; this test pins it down. The pre-fix behavior of +// json_free was a bare unique_ptr/shared_ptr reset that only dropped +// the caller's local reference; the parent container kept the subtree +// alive, so memory grew until the top-level parse completed. +TEST_CASE("json_free prunes a subtree from its parent", "[jsonpull][memory]") { + json_pull_ptr jp = json_begin_string("[[1, 2], [3, 4], [5, 6]]"); + + json_object_ptr outer; + int arrays_seen = 0; + + json_object_ptr j; + while ((j = json_read(jp)) != nullptr) { + if (j->type != JSON_ARRAY) { + continue; + } + arrays_seen++; + if (arrays_seen == 2) { + // This is [3, 4]; verify, then ask the parser to drop it. + REQUIRE(j->array().size() == 2); + REQUIRE(j->array()[0]->number() == 3); + REQUIRE(j->array()[1]->number() == 4); + json_free(j); + } else if (j->parent == nullptr) { + // The completed outer array. + outer = j; + break; + } + } + + REQUIRE(outer != nullptr); + REQUIRE(outer->type == JSON_ARRAY); + REQUIRE(outer->array().size() == 2); + + // First surviving element: [1, 2]. + REQUIRE(outer->array()[0]->type == JSON_ARRAY); + REQUIRE(outer->array()[0]->array().size() == 2); + REQUIRE(outer->array()[0]->array()[0]->number() == 1); + REQUIRE(outer->array()[0]->array()[1]->number() == 2); + + // Second surviving element (previously third): [5, 6]. + REQUIRE(outer->array()[1]->type == JSON_ARRAY); + REQUIRE(outer->array()[1]->array().size() == 2); + REQUIRE(outer->array()[1]->array()[0]->number() == 5); + REQUIRE(outer->array()[1]->array()[1]->number() == 6); +} + +// The companion case to the pruning test above: in a line-delimited +// stream, each feature returned by json_read is a top-level value +// with no parent, but the parser still co-owns it via jp->root. +// json_free must drop that parser reference too, otherwise the +// just-serialized feature stays in memory until the next feature +// starts parsing. +TEST_CASE("json_free releases a top-level value held by the parser", "[jsonpull][memory]") { + std::weak_ptr observer; + + json_pull_ptr jp = json_begin_string(R"({"a": 1, "b": [2, 3]})"); + json_object_ptr j = json_read_tree(jp); + REQUIRE(j != nullptr); + REQUIRE(j->type == JSON_HASH); + REQUIRE(j->parent == nullptr); + observer = j; + + json_free(j); + + REQUIRE(observer.expired()); +}