Review Board 1.7.16


Address JSON thread safety issues

Review Request #2716 - Created July 30, 2013 and submitted

David Lee
/trunk
Reviewers
asterisk-dev
kmoore, mjordan
Asterisk
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
Unit tests pass.
Total:
1
Open:
0
Resolved:
1
Dropped:
0
Status:
From:
Review request changed
Updated (Aug. 2, 2013, 10:20 a.m.)
  • changed from pending to submitted
Committed in revision 396132

https://reviewboard.asterisk.org/ runs on a server provided by Digium, Inc. and uses bandwidth donated to the open source Asterisk community by API Digital Communications in Huntsville, AL USA.
Please report problems with this site to asteriskteam@digium.com.