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

[David Lee]

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

(Updated Aug. 1, 2013, 9:05 a.m.)


Review request for Asterisk Developers, kmoore and Matt Jordan.


Changes
-------

Fixed ast_json_dump_string*() calls to free using ast_json_free


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 (updated)
-----

  /trunk/include/asterisk/json.h 395792 
  /trunk/main/cel.c 395792 
  /trunk/main/json.c 395792 
  /trunk/res/ari/ari_websockets.c 395792 
  /trunk/res/res_sorcery_astdb.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/20130801/050a4be9/attachment.htm>