Crash in dump() from a static object

[palacaze]

Change 29c5f32 from the performance regression bug #272 introduces a potential crash in dump(). This has to do with deinitialization order of static objects.

The following code (not json specific) reproduces the problem (tested on GNU/Linux 64 bits with GCC 5.4):

#include <locale>
#include <sstream>
#include <iostream>

namespace {
struct DecimalSeparator : std::numpunct<char> {
    char do_decimal_point() const { return '.'; }
};
}

struct test {
    void dump() {
        std::stringstream ss;
        const static std::locale loc(std::locale(), new DecimalSeparator);
        ss.imbue(loc);
        ss << "test " << 1.0;
        std::cout << ss.str() << std::endl;
    }
    ~test() {
        dump();
    }
};

static test t;

int main() {
    t.dump();
    return 0;
}

What happens here it that the static test is initialized first, then the first call to dump() creates the static locale object. When we leave main(), the static locale gets destroyed first (reversed initialization order), then the static test object is destructed, thus calling the destructor, which so happens to call dump() whose static object is no longer valid.

Removing the static keyword introduced in commit 29c5f32 fixes the crash.

[nlohmann]

@gregmarr Do you have an idea how to fix this without getting back to the worse performance?

[gregmarr]

The static is the entire reason for that commit. Removing it also returns the poor performance.
One fix is to not use the json library in the destructors of static objects, due to the indeterminate ordering.
If we don't want to impose that restriction, the only other way is to dynamically allocate that object and not free it. That may or may not resolve the issue, as json may depend on other things which have already been destroyed.

[palacaze]

Yes it may be a reasonable restriction. I think I can change my code to avoid calling dump in the destructor, but this assumption needs to be documented.

[gregmarr]

Avoiding code that relies on a particular order of global destruction is a good philosophy in general.

It's only dump(int) that has that direct restriction, but hash::operator() calls it, and so does an error handling path in patch(). There are two other static objects in the library. One is a constexpr std::array of a primitive type, and the other is a C array of a primitive type, so there are no destructors.

[gregmarr]

Avoiding code that relies on a particular order of global destruction is a good philosophy in general.

It's only dump(int) that has that direct restriction, but hash::operator() calls it, and so does an error handling path in patch(). There are two other static objects in the library. One is a constexpr std::array of a primitive type, and the other is a C array of a primitive type, so there are no destructors.

[gregmarr]

I haven't found anything else in here that has any dependencies on other global objects as long as the C++ standard library hasn't been terminate.

I did find another place where we might want to do the same thing as we did in dump(), around line 5900, and if we do decide to not free it, do the same here:

friend std::ostream& operator<<(std::ostream& o, const basic_json& j)
    {
    // fix locale problems
    const auto old_locale = o.imbue(std::locale(std::locale(), new DecimalSeparator));

[nlohmann]

Would it help change the loc object as static member of the class?

[gregmarr]

I don't think that will change when it is destroyed. The problem is that it is destroyed at all. By having a static object, the destructor has to run for it at some point. Once that happens, any calls to the library from the destructor of another static object will access uninitialized memory. The way to avoid that is to make it a static pointer instead, so that the destructor never runs.

[gregmarr]

Example, untested:

// This is intentionally not deleted so that the library will continue to work in the
// destructors of static objects, up to the point where the C++ standard library is
// shut down.
const static std::locale *loc = new std::locale(std::locale(), new DecimalSeparator);
ss.imbue(*loc);

[palacaze]

I can confirm the proposed alternative approach to the static keyword fixes the crash for me.

[nlohmann]

But wouldn't this mean leaking memory which would also be worth noting in the README?

[gregmarr]

Yes, it would be memory that would not be freed on exit, and it should be noted in the README as well as in a comment above it.

[palacaze]

If you care about minimizing those, you may want to share the same static instance, through a static private function like in the basic_json class like so:

// This is intentionally not deleted so that the library will continue to work in the
// destructors of static objects, up to the point where the C++ standard library is
// shut down.
const std::locale & locale_with_fixed_decimal() {
    const static std::locale *loc = new std::locale(std::locale(), new DecimalSeparator);
    return *loc;
}

and use it in both bump() and operator<<().

[gregmarr]

or even move it out of the class and into the namespace.

[TurpentineDistillery]

Maybe instead of looking for the correct way to solve the problem just avoid it altogether by yanking the whole DecimalSeparator business and imbue with std::locale::classic() ?

[gregmarr]

@TurpentineDistillery Oooh, much better. I had never run into that one before.

[nlohmann]

Thanks @TurpentineDistillery for hinting to std::locale::classic()!

With 43dbe02 I changed the code accordingly. The performance of dump remains the same:

[~/.../Repositories/json] ./json_benchmark_before --bench dump.* --benchtime 10
dump jeopardy.json                           10  1026884239 ns/op
dump jeopardy.json with indent               10  1273794859 ns/op
./json_benchmark_before 51.704s

[~/.../Repositories/json] ./json_benchmark_after --bench dump.* --benchtime 10
dump jeopardy.json                           10  1056578109 ns/op
dump jeopardy.json with indent               10  1231761467 ns/op
./json_benchmark_after 51.134s

@palacaze Does this fix your problem?

[palacaze]

I can't reproduce the crash with this change, so as far as I am concerned the bug has been fixed.

[nlohmann]

Thanks for the quick feedback! I shall merge the feature branch once the Travis builds have completed.

Thanks everyone!