[asterisk-dev] [Code Review] 2716: Address JSON thread safety issues

[Mark Michelson]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2716/#review9256
-----------------------------------------------------------

Ship it!


Excepting opticron's finding of course.

- Mark Michelson


On July 30, 2013, 8:39 p.m., David Lee wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2716/
> -----------------------------------------------------------
> 
> (Updated July 30, 2013, 8:39 p.m.)
> 
> 
> Review request for Asterisk Developers, kmoore and Matt Jordan.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> In tracking down some unit tests failures, I ended up reading the fine
> print[1] regarding Jansson's thread safety.
> 
> In short:
>  1. Ref-counting is non-atomic.
>  2. json_dumps() and friends are not thread safe.
> 
> This patch adds locking where necessary to our ast_json_* wrapper API,
> with documentation in json.h describing the thread safety limitations of
> the API.
> 
>  * Jansson (as of 2.4) provides fairly weak thread safety guarantees. The
>  * Asterisk wrapper improves upon that slightly. The remaining refcounting
>  * problems are issues when slicing/sharing/mixing instances between JSON
>  * objects and arrays, which we avoid.
>  *
>  * The \c ast_json_dump_* functions are thread safe for multiple concurrent
>  * dumps of the same object, so long as the concurrent dumps start from the same
>  * \c root object. But if an object is shared by other JSON objects/arrays, then
>  * concurrent dumps of the outer objects/arrays are not thread safe. This can be
>  * avoided by using ast_json_deep_copy() when sharing JSON instances between
>  * objects.
>  *
>  * The ast_json_ref() and ast_json_unref() functions are thread safe. Since the
>  * Asterisk wrapper exclusively uses the reference stealing API, Jansson won't
>  * be performing many refcount modifications behind our backs. There are a few
>  * exceptions.
>  *
>  * The first is the transitive json_decref() that occurs when \ref
>  * AST_JSON_OBJECT and \ref AST_JSON_ARRAY instances are deleted. This can be
>  * avoided by using ast_json_deep_copy() when sharing JSON instances between
>  * objects.
>  *
>  * The second is when using the reference borrowing specifier in
>  * ast_json_pack() (capital \c O). This can be avoided by using the reference
>  * stealing specifier (lowercase \c o) and wrapping the JSON object parameter
>  * with ast_json_ref() for an explicit ref-bump.
> 
>  [1]: http://www.digip.org/jansson/doc/2.4/portability.html#thread-safety
> 
> 
> Diffs
> -----
> 
>   /trunk/include/asterisk/json.h 395792 
>   /trunk/main/json.c 395792 
>   /trunk/tests/test_json.c 395792 
> 
> Diff: https://reviewboard.asterisk.org/r/2716/diff/
> 
> 
> Testing
> -------
> 
> Unit tests pass.
> 
> 
> Thanks,
> 
> David Lee
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130731/4ca195a5/attachment.htm>