Code review comment for lp:~kamstrup/couchdb-glib/couchdb-glib-gtkdoc

Revision history for this message
Tim Cole (tcole) wrote :

The refcounting logic is unsafe as written; correct refcounting in the multithreaded case can be subtle and not something I would recommend attempting on your own.

(Is there a reason to use a boxed type rather than deriving from gobject, which has correct refcounting?)

The two biggest thread-related issues are with _unref; first, there is a race between the read of the refcount and acting on it, and secondly g_atomic_int_compare_and_exchange is misused here, since it can potentially fail and is supposed to be used in a retrying loop.

The requirements for refcounting here are at least a little simpler than gobject's (since I don't think you have to worry about e.g. revivification or weak/floating refs); if you were to continue with a boxed type, a correct implementation for your simple case would look something like this:

 CouchDBDatabaseInfo *
 couchdb_database_info_ref (CouchDBDatabaseInfo *dbinfo)
 {
  g_return_val_if_fail (dbinfo != NULL, NULL);
  g_return_val_if_fail (dbinfo->ref_count > 0, NULL);

  g_atomic_int_inc(&dbinfo->ref_count);

  return dbinfo;
 }

 void
 couchdb_database_info_unref (CouchDBDatabaseInfo *dbinfo)
 {
  gboolean is_zero;

  g_return_if_fail (dbinfo != NULL);
  g_return_if_fail (dbinfo->ref_count > 0);

  is_zero = g_atomic_int_dec_and_test(&dbinfo->ref_count);

  if (is_zero) {
   g_free (dbinfo->dbname);
   g_slice_free (CouchDBDatabaseInfo, dbinfo);
  }
 }

(Also, is single-space-indent acceptable from a CodingStyle point of view?)

review: Needs Fixing

« Back to merge proposal