diff --git a/.github/workflows/ubuntu.yml b/.github/workflows/ubuntu.yml index 2ef56a80b..107860ac6 100644 --- a/.github/workflows/ubuntu.yml +++ b/.github/workflows/ubuntu.yml @@ -100,7 +100,7 @@ jobs: container: ubuntu:focal strategy: matrix: - target: [ci_cmake_flags, ci_test_diagnostics, ci_test_diagnostic_positions, ci_test_noexceptions, ci_test_noimplicitconversions, ci_test_legacycomparison, ci_test_noglobaludls] + target: [ci_cmake_flags, ci_test_diagnostics, ci_test_diagnostic_positions, ci_test_noexceptions, ci_test_noimplicitconversions, ci_test_legacycomparison, ci_test_noglobaludls, ci_test_no_thread_local] steps: - name: Install build-essential run: apt-get update ; apt-get install -y build-essential unzip wget git libssl-dev diff --git a/cmake/ci.cmake b/cmake/ci.cmake index 6b1d325d8..011c552fd 100644 --- a/cmake/ci.cmake +++ b/cmake/ci.cmake @@ -242,6 +242,25 @@ add_custom_target(ci_test_noglobaludls COMMENT "Compile and test with global UDLs disabled" ) +############################################################################### +# Disable thread-local storage. +############################################################################### + +# Without thread-local storage, the copy constructor cannot bound its descent +# and copies every object and array without the call stack. That path is +# otherwise only reached by values nested deeper than the bound, so this target +# is what runs the whole test suite through it. +add_custom_target(ci_test_no_thread_local + COMMAND ${CMAKE_COMMAND} + -DCMAKE_BUILD_TYPE=Debug -GNinja + -DJSON_BuildTests=ON + -DCMAKE_CXX_FLAGS=-DJSON_NO_THREAD_LOCAL + -S${PROJECT_SOURCE_DIR} -B${PROJECT_BINARY_DIR}/build_no_thread_local + COMMAND ${CMAKE_COMMAND} --build ${PROJECT_BINARY_DIR}/build_no_thread_local + COMMAND cd ${PROJECT_BINARY_DIR}/build_no_thread_local && ${CMAKE_CTEST_COMMAND} --parallel ${N} --output-on-failure + COMMENT "Compile and test without thread-local storage" +) + ############################################################################### # Coverage. ############################################################################### diff --git a/docs/mkdocs/docs/api/macros/json_no_thread_local.md b/docs/mkdocs/docs/api/macros/json_no_thread_local.md index 6d6460811..d98745243 100644 --- a/docs/mkdocs/docs/api/macros/json_no_thread_local.md +++ b/docs/mkdocs/docs/api/macros/json_no_thread_local.md @@ -14,7 +14,9 @@ shared between threads would be raced. Without that counter, no descent can be bounded safely, so objects and arrays are copied without the call stack right away. Copying keeps working exactly as it does otherwise - the same values come out, and deeply nested values are copied -just as safely - but copying is measurably slower, as the containers no longer copy themselves. +just as safely - but copying is slower, because the containers no longer copy themselves. Copying the benchmark +documents takes 9% (`canada.json`) to 34% (`twitter.json`) longer; values built mostly from objects are affected the +most. ## Default definition diff --git a/tests/src/unit-diagnostic-positions.cpp b/tests/src/unit-diagnostic-positions.cpp index ad9527540..7f1471d5e 100644 --- a/tests/src/unit-diagnostic-positions.cpp +++ b/tests/src/unit-diagnostic-positions.cpp @@ -68,6 +68,61 @@ TEST_CASE("Better diagnostics with positions") CHECK(j.end_pos() == root.size()); } + SECTION("copying keeps the positions of nested values (#5387)") + { + // Values nested deeper than the copy constructor's descent bound are + // copied without the call stack, on a path that has to carry the + // positions over itself; shallower ones copy their containers, which + // bring the positions along. Both sides of the bound are checked here. + const auto check_copy = [](std::size_t depth, bool objects) + { + CAPTURE(depth) + CAPTURE(objects) + + const std::string open = objects ? R"({"a":)" : "["; + const std::string close = objects ? "}" : "]"; + + std::string text; + for (std::size_t i = 0; i < depth; ++i) + { + text += open; + } + text += "12"; + for (std::size_t i = 0; i < depth; ++i) + { + text += close; + } + + const json original = json::parse(text); + const json copy(original); // NOLINT(performance-unnecessary-copy-initialization) + + const json* o = &original; + const json* c = © + for (std::size_t level = 0; level <= depth; ++level) + { + CAPTURE(level) + REQUIRE(c->start_pos() == o->start_pos()); + REQUIRE(c->end_pos() == o->end_pos()); + + if (level < depth) + { + o = objects ? &o->at("a") : &o->at(0); + c = objects ? &c->at("a") : &c->at(0); + } + } + }; + + const bool shapes[] = {false, true}; + for (const bool objects : shapes) + { + check_copy(1, objects); + check_copy(127, objects); + check_copy(128, objects); + check_copy(129, objects); + check_copy(300, objects); + } + } + SECTION("JSON patch add to primitive parent (#4292)") { // the JSON Patch "add" target /foo/bar/baz has a string parent diff --git a/tests/src/unit-diagnostics.cpp b/tests/src/unit-diagnostics.cpp index 1e8ed6aa3..1ceb6365b 100644 --- a/tests/src/unit-diagnostics.cpp +++ b/tests/src/unit-diagnostics.cpp @@ -273,5 +273,62 @@ TEST_CASE("Regression tests for extended diagnostics") CHECK(j1["numbers"]["two"] == 2); CHECK(j1["string"] == "t"); } + + SECTION("Regression test for issue #5387 - copying keeps the parents of nested values") + { + // A value nested deeper than the copy constructor's descent bound is + // copied without the call stack. Every container that path creates has + // to have the parents of its children set, or the JSON Pointer in the + // diagnostic is cut short. + const std::size_t depth = 300; + + SECTION("objects") + { + json j = "not a number"; + std::string pointer; + for (std::size_t i = 0; i < depth; ++i) + { + j = json{{"a", j}}; + pointer += "/a"; + } + + json const copy(j); // NOLINT(performance-unnecessary-copy-initialization) + + const json* inner = © + for (std::size_t i = 0; i < depth; ++i) + { + inner = &inner->at("a"); + } + + std::string const expected = "[json.exception.type_error.302] (" + pointer + ") type must be number, but is string"; + int i = 0; + CHECK_THROWS_WITH_AS(i = inner->get(), expected.c_str(), json::type_error); + CHECK(i == 0); + } + + SECTION("arrays") + { + json j = "not a number"; + std::string pointer; + for (std::size_t i = 0; i < depth; ++i) + { + j = json::array({j}); + pointer += "/0"; + } + + json const copy(j); // NOLINT(performance-unnecessary-copy-initialization) + + const json* inner = © + for (std::size_t i = 0; i < depth; ++i) + { + inner = &inner->at(0); + } + + std::string const expected = "[json.exception.type_error.302] (" + pointer + ") type must be number, but is string"; + int i = 0; + CHECK_THROWS_WITH_AS(i = inner->get(), expected.c_str(), json::type_error); + CHECK(i == 0); + } + } }