Fix ~basic_json causing std::terminate

[Ignition]

During ~basic_json, it was possible to receive an exception from the allocation subsystem (quota/policy violation etc). This would cause std::terminate because destructors are noexcept. This fix will gracefully handle that case by allowing RAII to handle anything within the current stack (used to avoid recursion), then fallback to the recursion approach for anything that still remains.

fixes #4653

[coveralls]

coverage: 99.17% (-0.02%) from 99.186% when pulling 810328368547e918a52161314dcd2a0a2c8a10ed on Ignition:fix_4653 into 0b6881a95f8aaf60bd2324ecdf2258a65bf33baf on nlohmann:develop.

[github-actions[bot]]

🔴 Amalgamation check failed! 🔴

The source code has not been amalgamated. @Ignition Please read and follow the Contribution Guidelines.

[Ignition]

@nlohmann I'm having trouble making the Windows build correct. I don't have windows machine available to debug/diagnose.

[nlohmann]

@nlohmann I'm having trouble making the Windows build correct. I don't have windows machine available to debug/diagnose.

I don't use Windows myself. :-/

[gregmarr]

I'm not wild about this idea. It's swallowing unknown exceptions and setting up a stack overflow.

[nlohmann]

I am not sure what to make of this right now. The noexcept annotation was from times where we did hardly anything in the destructor.

[gregmarr]

Destructors are noexcept by default.

[Ignition]

I'm not wild about this idea. It's swallowing unknown exceptions and setting up a stack overflow.

Normally I would fully agree about swallowing unknown exceptions being bad. But in this case with the current implementation there are only a few potential sources of exceptions.

• allocation subsystem, should only be able to throw an exception via allocate or construct, I have yet to see a use case where destruct or deallocate throw (for the same reasons that destructors are noexcept)
• iteration, which doesn't ATM throw, and a good implementation shouldn't ever need to throw
• moves, which are noexcept so no exceptions there either

So it just a question about is it safe to swallow unknown exception from the allocation subsystem. My feeling is yes, because its actually not swallowing, but handling a corner case even if you don't know what that exception is.

You either:

• Do nothing with the exception, hit noexcept, std::terminate which kills the program which is bad.
• Make ~basic_json noexcept(false), which I DO NOT recommend. It just moves the problem onto the consumer, any type that has json as a data member would also need a noexcept(false) destructor.
• Handle the unknown exception, in this case we are confident the exception could have only come from the memory subsystem. Regardless of the actual exception, we can handle it by letting RAII do it job and then fallback to doing regular recursive cleanup. This does risk stack exhaustion, but that is much better than std::terminate.

[gregmarr]

Do you use the JSON_DIAGNOSTICS define? If not, would you be okay with using that option?

I think it's possible to rewrite this to not use the vector if the m_parent field exists, using an algorithm based on Morris Traversal. The basic idea is that you have a current pointer. You start at the root (this), and walk the current pointer down the tree. When you find an object/array, you look at the first object element or the last array element. If it's a leaf node, remove it from the object/array. By using the last array element instead of the first, you are just doing a pop_back on the array instead of moving everything after it. If it's an object/array, you update current and repeat. Once the object/array is empty, you update current to be its parent, and then remove the thing you just processed from its object/array.

I haven't written any of this code, just thinking about what might be possible.

[Ignition]

@gregmarr @nlohmann I have delivered a patch in my build that prevents std::terminate in my product. Unfortunate I don't have the time to spend on getting this merged here. I maybe will come back to this PR, but don't bet/rely on it.

@gregmarr I agree it is possible to do it without heap allocation. I think that modification is worth doing (assuming benchmarks show that is fine). Here is Herb Sutter talking about destroying trees https://youtu.be/JfmTagWcqoE?si=XyJR0YpYZGm4FGbB&t=970

[gregmarr]

@Ignition Yeah, the current method is the bottom row of the table, O(N) heap space with moves. In order to remove the O(N) heap space, which is the issue that you're having of allocating memory in a destructor, we'd have to go to the second row of the table, which is O(N log N) time.

[github-actions[bot]]

This pull request has been marked as stale because it has had no activity for 30 days. While we won’t close it automatically, we encourage you to update or comment if it is still relevant. Keeping pull requests active and up-to-date helps us review and merge changes more efficiently. Thank you for your contributions!