search

danielaparker / jsoncons

A C++, header-only library for constructing JSON and JSON-like data formats, with JSON Pointer, JSON Patch, JSON Schema, JSONPath, JMESPath, CSV, MessagePack, CBOR, BSON, UBJSON
https://danielaparker.github.io/jsoncons
Other
699 stars 158 forks source link

const object& object_value() const isn't const #428

Closed ergpudb closed 1 year ago

ergpudb commented 1 year ago

Calling object_value() on an empty object (storage kind = empty_object_value) changes the value, even in the const overload of the method. This would seem to make basic_json not thread-safe for simultaneous read-only access, at least via that route -- I'm not sure if that is the intention, since the docs don't specifically mention thread safety anywhere, but it's not unreasonable to assume that it would be okay in the absence of a warning against it, so at the very least this seems like a significant "gotcha".

v0.170.0

danielaparker commented 1 year ago

Thanks for raising this issue.

For the const version, const object& object_value() const, I don't believe it will ever be called with json_storage_kind::empty_object_value, so I think we can safely remove the troublesome case from the switch statement.

When attempting to mutate a basic_json empty object value, we need to implicitly convert it into an object value first. However, we can probably reduce the scope of that, and have it take place only within mutators, rather than in object_value().

ergpudb commented 1 year ago

Actually I found this because of a call to object_value() on a const basic_json (maybe I'm not using something correctly)? For my application I am carefully tracking memory usage and noticed that after doing this I was getting different memory readings than beforehand, despite only touching const objects.

ergpudb commented 1 year ago

To elaborate, I had an empty basic_json; is_object() returned true, so then I was using object_value() to access the keys. I wouldn't expect any keys to be there in that instance, but it silently allocated a vector underneath.

danielaparker commented 1 year ago

object_value() should be private, it's implementation detail, not intended to be called by a user.

ergpudb commented 1 year ago

Ah ok, user error. (I assume object_range() is what I'm looking for.)

Can it be made private? Now I know not to call it, and if I don't call it the initial issue wouldn't happen... but it seems like a somewhat obvious thing to call based on the name.

danielaparker commented 1 year ago

Yes, object_range() works for all objects, regardless of internal storage.

And yes, we can make object_value() private.

danielaparker commented 1 year ago

object_value() has been removed in 0.171.0