Merge pull request #389 from coreyfarrell/threadsafety

Atomic references and thread safe json_dump

CMakeLists.txt

@@ -303,8 +303,8 @@ else()
    set (JSON_INLINE)
 endif()
 
-check_c_source_compiles ("int main() { unsigned long val; __sync_bool_compare_and_swap(&val, 0, 1); return 0; } " HAVE_SYNC_BUILTINS)
-check_c_source_compiles ("int main() { char l; unsigned long v; __atomic_test_and_set(&l, __ATOMIC_RELAXED); __atomic_store_n(&v, 1, __ATOMIC_RELEASE); __atomic_load_n(&v, __ATOMIC_ACQUIRE); return 0; }" HAVE_ATOMIC_BUILTINS)
+check_c_source_compiles ("int main() { unsigned long val; __sync_bool_compare_and_swap(&val, 0, 1); __sync_add_and_fetch(&val, 1); __sync_sub_and_fetch(&val, 1); return 0; } " HAVE_SYNC_BUILTINS)
+check_c_source_compiles ("int main() { char l; unsigned long v; __atomic_test_and_set(&l, __ATOMIC_RELAXED); __atomic_store_n(&v, 1, __ATOMIC_RELEASE); __atomic_load_n(&v, __ATOMIC_ACQUIRE); __atomic_add_fetch(&v, 1, __ATOMIC_ACQUIRE); __atomic_sub_fetch(&v, 1, __ATOMIC_RELEASE); return 0; }" HAVE_ATOMIC_BUILTINS)
 
 set (JANSSON_INITIAL_HASHTABLE_ORDER 3 CACHE STRING "Number of buckets new object hashtables contain is 2 raised to this power. The default is 3, so empty hashtables contain 2^3 = 8 buckets.")
 

configure.ac

@@ -38,25 +38,33 @@ AC_CHECK_FUNCS([close getpid gettimeofday localeconv open read sched_yield strto
 AC_MSG_CHECKING([for gcc __sync builtins])
 have_sync_builtins=no
 AC_TRY_LINK(
-  [], [unsigned long val; __sync_bool_compare_and_swap(&val, 0, 1);],
+  [], [unsigned long val; __sync_bool_compare_and_swap(&val, 0, 1); __sync_add_and_fetch(&val, 1); __sync_sub_and_fetch(&val, 1);],
   [have_sync_builtins=yes],
 )
 if test "x$have_sync_builtins" = "xyes"; then
   AC_DEFINE([HAVE_SYNC_BUILTINS], [1],
     [Define to 1 if gcc's __sync builtins are available])
+  json_have_sync_builtins=1
+else
+  json_have_sync_builtins=0
 fi
+AC_SUBST([json_have_sync_builtins])
 AC_MSG_RESULT([$have_sync_builtins])
 
 AC_MSG_CHECKING([for gcc __atomic builtins])
 have_atomic_builtins=no
 AC_TRY_LINK(
-  [], [char l; unsigned long v; __atomic_test_and_set(&l, __ATOMIC_RELAXED); __atomic_store_n(&v, 1, __ATOMIC_RELEASE); __atomic_load_n(&v, __ATOMIC_ACQUIRE);],
+  [], [char l; unsigned long v; __atomic_test_and_set(&l, __ATOMIC_RELAXED); __atomic_store_n(&v, 1, __ATOMIC_RELEASE); __atomic_load_n(&v, __ATOMIC_ACQUIRE); __atomic_add_fetch(&v, 1, __ATOMIC_ACQUIRE); __atomic_sub_fetch(&v, 1, __ATOMIC_RELEASE);],
   [have_atomic_builtins=yes],
 )
 if test "x$have_atomic_builtins" = "xyes"; then
   AC_DEFINE([HAVE_ATOMIC_BUILTINS], [1],
     [Define to 1 if gcc's __atomic builtins are available])
+  json_have_atomic_builtins=1
+else
+  json_have_atomic_builtins=0
 fi
+AC_SUBST([json_have_atomic_builtins])
 AC_MSG_RESULT([$have_atomic_builtins])
 
 case "$ac_cv_type_long_long_int$ac_cv_func_strtoll" in

doc/apiref.rst

@@ -58,6 +58,11 @@ the library:
       /* Code specific to version 1.3 and above */
       #endif
 
+``JANSSON_THREAD_SAFE_REFCOUNT``
+  If this value is defined all read-only operations and reference counting in
+  Jansson are thread safe.  This value is not defined for versions older than
+  ``2.11`` or when the compiler does not provide built-in atomic functions.
+
 
 Value Representation
 ====================

doc/portability.rst

@@ -7,29 +7,31 @@ Portability
 Thread safety
 -------------
 
-Jansson is thread safe and has no mutable global state. The only
-exceptions are the hash function seed and memory allocation functions,
-see below.
+Jansson as a library is thread safe and has no mutable global state.
+The only exceptions are the hash function seed and memory allocation
+functions, see below.
 
-There's no locking performed inside Jansson's code, so a multithreaded
-program must perform its own locking if JSON values are shared by
-multiple threads. Jansson's reference counting semantics may make this
-a bit harder than it seems, as it's possible to have a reference to a
-value that's also stored inside a list or object. Modifying the
-container (adding or removing values) may trigger concurrent access to
-such values, as containers manage the reference count of their
-contained values. Bugs involving concurrent incrementing or
-decrementing of deference counts may be hard to track.
+There's no locking performed inside Jansson's code. **Read-only**
+access to JSON values shared by multiple threads is safe, but
+**mutating** a JSON value that's shared by multiple threads is not. A
+multithreaded program must perform its own locking if JSON values
+shared by multiple threads are mutated.
 
-The encoding functions (:func:`json_dumps()` and friends) track
-reference loops by modifying the internal state of objects and arrays.
-For this reason, encoding functions must not be run on the same JSON
-values in two separate threads at the same time. As already noted
-above, be especially careful if two arrays or objects share their
-contained values with another array or object.
+However, **reference count manipulation** (:func:`json_incref()`,
+:func:`json_decref()`) is usually thread-safe, and can be performed on
+JSON values that are shared among threads. The thread-safety of
+reference counting can be checked with the
+``JANSSON_THREAD_SAFE_REFCOUNT`` preprocessor constant. Thread-safe
+reference count manipulation is achieved using compiler built-in
+atomic functions, which are available in most modern compilers.
 
-If you want to make sure that two JSON value hierarchies do not
-contain shared values, use :func:`json_deep_copy()` to make copies.
+If compiler support is not available (``JANSSON_THREAD_SAFE_REFCOUNT``
+is not defined), it may be very difficult to ensure thread safety of
+reference counting. It's possible to have a reference to a value
+that's also stored inside an array or object in another thread.
+Modifying the container (adding or removing values) may trigger
+concurrent access to such values, as containers manage the reference
+count of their contained values.
 
 
 Hash function seed

src/dump.c

@@ -196,8 +196,17 @@ static int compare_keys(const void *key1, const void *key2)
     return strcmp(*(const char **)key1, *(const char **)key2);
 }
 
+static int loop_check(hashtable_t *parents, const json_t *json, char *key, size_t key_size)
+{
+    snprintf(key, key_size, "%p", json);
+    if (hashtable_get(parents, key))
+        return -1;
+
+    return hashtable_set(parents, key, json_null());
+}
+
 static int do_dump(const json_t *json, size_t flags, int depth,
-                   json_dump_callback_t dump, void *data)
+                   hashtable_t *parents, json_dump_callback_t dump, void *data)
 {
     int embed = flags & JSON_EMBED;
 
@@ -251,58 +260,53 @@ static int do_dump(const json_t *json, size_t flags, int depth,
         {
             size_t n;
             size_t i;
-
-            json_array_t *array;
+            /* Space for "0x", double the sizeof a pointer for the hex and a terminator. */
+            char key[2 + (sizeof(json) * 2) + 1];
 
             /* detect circular references */
-            array = json_to_array(json);
-            if(array->visited)
-                goto array_error;
-            array->visited = 1;
+            if (loop_check(parents, json, key, sizeof(key)))
+                return -1;
 
             n = json_array_size(json);
 
             if(!embed && dump("[", 1, data))
-                goto array_error;
+                return -1;
             if(n == 0) {
-                array->visited = 0;
+                hashtable_del(parents, key);
                 return embed ? 0 : dump("]", 1, data);
             }
             if(dump_indent(flags, depth + 1, 0, dump, data))
-                goto array_error;
+                return -1;
 
             for(i = 0; i < n; ++i) {
                 if(do_dump(json_array_get(json, i), flags, depth + 1,
-                           dump, data))
-                    goto array_error;
+                           parents, dump, data))
+                    return -1;
 
                 if(i < n - 1)
                 {
                     if(dump(",", 1, data) ||
                        dump_indent(flags, depth + 1, 1, dump, data))
-                        goto array_error;
+                        return -1;
                 }
                 else
                 {
                     if(dump_indent(flags, depth, 0, dump, data))
-                        goto array_error;
+                        return -1;
                 }
             }
 
-            array->visited = 0;
+            hashtable_del(parents, key);
             return embed ? 0 : dump("]", 1, data);
-
-        array_error:
-            array->visited = 0;
-            return -1;
         }
 
         case JSON_OBJECT:
         {
-            json_object_t *object;
             void *iter;
             const char *separator;
             int separator_length;
+            /* Space for "0x", double the sizeof a pointer for the hex and a terminator. */
+            char key[2 + (sizeof(json) * 2) + 1];
 
             if(flags & JSON_COMPACT) {
                 separator = ":";
@@ -314,21 +318,19 @@ static int do_dump(const json_t *json, size_t flags, int depth,
             }
 
             /* detect circular references */
-            object = json_to_object(json);
-            if(object->visited)
-                goto object_error;
-            object->visited = 1;
+            if (loop_check(parents, json, key, sizeof(key)))
+                return -1;
 
             iter = json_object_iter((json_t *)json);
 
             if(!embed && dump("{", 1, data))
-                goto object_error;
+                return -1;
             if(!iter) {
-                object->visited = 0;
+                hashtable_del(parents, key);
                 return embed ? 0 : dump("}", 1, data);
             }
             if(dump_indent(flags, depth + 1, 0, dump, data))
-                goto object_error;
+                return -1;
 
             if(flags & JSON_SORT_KEYS)
             {
@@ -338,7 +340,7 @@ static int do_dump(const json_t *json, size_t flags, int depth,
                 size = json_object_size(json);
                 keys = jsonp_malloc(size * sizeof(const char *));
                 if(!keys)
-                    goto object_error;
+                    return -1;
 
                 i = 0;
                 while(iter)
@@ -362,10 +364,10 @@ static int do_dump(const json_t *json, size_t flags, int depth,
 
                     dump_string(key, strlen(key), dump, data, flags);
                     if(dump(separator, separator_length, data) ||
-                       do_dump(value, flags, depth + 1, dump, data))
+                       do_dump(value, flags, depth + 1, parents, dump, data))
                     {
                         jsonp_free(keys);
-                        goto object_error;
+                        return -1;
                     }
 
                     if(i < size - 1)
@@ -374,7 +376,7 @@ static int do_dump(const json_t *json, size_t flags, int depth,
                            dump_indent(flags, depth + 1, 1, dump, data))
                         {
                             jsonp_free(keys);
-                            goto object_error;
+                            return -1;
                         }
                     }
                     else
@@ -382,7 +384,7 @@ static int do_dump(const json_t *json, size_t flags, int depth,
                         if(dump_indent(flags, depth, 0, dump, data))
                         {
                             jsonp_free(keys);
-                            goto object_error;
+                            return -1;
                         }
                     }
                 }
@@ -401,31 +403,27 @@ static int do_dump(const json_t *json, size_t flags, int depth,
                     dump_string(key, strlen(key), dump, data, flags);
                     if(dump(separator, separator_length, data) ||
                        do_dump(json_object_iter_value(iter), flags, depth + 1,
-                               dump, data))
-                        goto object_error;
+                               parents, dump, data))
+                        return -1;
 
                     if(next)
                     {
                         if(dump(",", 1, data) ||
                            dump_indent(flags, depth + 1, 1, dump, data))
-                            goto object_error;
+                            return -1;
                     }
                     else
                     {
                         if(dump_indent(flags, depth, 0, dump, data))
-                            goto object_error;
+                            return -1;
                     }
 
                     iter = next;
                 }
             }
 
-            object->visited = 0;
+            hashtable_del(parents, key);
             return embed ? 0 : dump("}", 1, data);
-
-        object_error:
-            object->visited = 0;
-            return -1;
         }
 
         default:
@@ -489,10 +487,18 @@ int json_dump_file(const json_t *json, const char *path, size_t flags)
 
 int json_dump_callback(const json_t *json, json_dump_callback_t callback, void *data, size_t flags)
 {
+    int res;
+    hashtable_t parents_set;
+
     if(!(flags & JSON_ENCODE_ANY)) {
         if(!json_is_array(json) && !json_is_object(json))
            return -1;
     }
 
-    return do_dump(json, flags, 0, callback, data);
+    if (hashtable_init(&parents_set))
+        return -1;
+    res = do_dump(json, flags, 0, &parents_set, callback, data);
+    hashtable_close(&parents_set);
+
+    return res;
 }

src/jansson.h

@@ -33,6 +33,11 @@ extern "C" {
                               (JANSSON_MINOR_VERSION << 8)  |   \
                               (JANSSON_MICRO_VERSION << 0))
 
+/* If __atomic or __sync builtins are available the library is thread
+ * safe for all read-only functions plus reference counting. */
+#if JSON_HAVE_ATOMIC_BUILTINS || JSON_HAVE_SYNC_BUILTINS
+#define JANSSON_THREAD_SAFE_REFCOUNT 1
+#endif
 
 /* types */
 
@@ -49,7 +54,7 @@ typedef enum {
 
 typedef struct json_t {
     json_type type;
-    size_t refcount;
+    volatile size_t refcount;
 } json_t;
 
 #ifndef JANSSON_USING_CMAKE /* disabled if using cmake */
@@ -94,11 +99,23 @@ json_t *json_false(void);
 #define json_boolean(val)      ((val) ? json_true() : json_false())
 json_t *json_null(void);
 
+/* do not call JSON_INTERNAL_INCREF or JSON_INTERNAL_DECREF directly */
+#if JSON_HAVE_ATOMIC_BUILTINS
+#define JSON_INTERNAL_INCREF(json) __atomic_add_fetch(&json->refcount, 1, __ATOMIC_ACQUIRE)
+#define JSON_INTERNAL_DECREF(json) __atomic_sub_fetch(&json->refcount, 1, __ATOMIC_RELEASE)
+#elif JSON_HAVE_SYNC_BUILTINS
+#define JSON_INTERNAL_INCREF(json) __sync_add_and_fetch(&json->refcount, 1)
+#define JSON_INTERNAL_DECREF(json) __sync_sub_and_fetch(&json->refcount, 1)
+#else
+#define JSON_INTERNAL_INCREF(json) (++json->refcount)
+#define JSON_INTERNAL_DECREF(json) (--json->refcount)
+#endif
+
 static JSON_INLINE
 json_t *json_incref(json_t *json)
 {
     if(json && json->refcount != (size_t)-1)
-        ++json->refcount;
+        JSON_INTERNAL_INCREF(json);
     return json;
 }
 
@@ -108,7 +125,7 @@ void json_delete(json_t *json);
 static JSON_INLINE
 void json_decref(json_t *json)
 {
-    if(json && json->refcount != (size_t)-1 && --json->refcount == 0)
+    if(json && json->refcount != (size_t)-1 && JSON_INTERNAL_DECREF(json) == 0)
         json_delete(json);
 }
 

src/jansson_config.h.in

@@ -36,6 +36,14 @@
    otherwise to 0. */
 #define JSON_HAVE_LOCALECONV @json_have_localeconv@
 
+/* If __atomic builtins are available they will be used to manage
+   reference counts of json_t. */
+#define JSON_HAVE_ATOMIC_BUILTINS @json_have_atomic_builtins@
+
+/* If __atomic builtins are not available we try using __sync builtins
+   to manage reference counts of json_t. */
+#define JSON_HAVE_SYNC_BUILTINS @json_have_sync_builtins@
+
 /* Maximum recursion depth for parsing JSON input.
    This limits the depth of e.g. array-within-array constructions. */
 #define JSON_PARSER_MAX_DEPTH 2048

src/jansson_private.h

@@ -35,7 +35,6 @@
 typedef struct {
     json_t json;
     hashtable_t hashtable;
-    int visited;
 } json_object_t;
 
 typedef struct {
@@ -43,7 +42,6 @@ typedef struct {
     size_t size;
     size_t entries;
     json_t **table;
-    int visited;
 } json_array_t;
 
 typedef struct {

src/value.c

@@ -67,8 +67,6 @@ json_t *json_object(void)
         return NULL;
     }
 
-    object->visited = 0;
-
     return &object->json;
 }
 
@@ -354,8 +352,6 @@ json_t *json_array(void)
         return NULL;
     }
 
-    array->visited = 0;
-
     return &array->json;
 }