After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 650459 - hash table consistency while calling destroy notify funcs
hash table consistency while calling destroy notify funcs
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2011-05-18 05:35 UTC by Behdad Esfahbod
Modified: 2012-08-28 01:59 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Behdad Esfahbod 2011-05-18 05:35:17 UTC
It's desirable that a destroy_notify callback be able to modify the hashtable.  For example, in cairo, we have a legitimate usecase that a destroy_notify removes another item from the hashtable.

Checking the code, I don't think we currently handle that correctly:

static void 
g_hash_table_remove_node (GHashTable   *hash_table, 
                          int           i, 
                          gboolean      notify) 
{ 
  if (notify && hash_table->key_destroy_func) 
    hash_table->key_destroy_func (hash_table->keys[i]); 
 
  if (notify && hash_table->value_destroy_func) 
    hash_table->value_destroy_func (hash_table->values[i]); 
 
  /* Erect tombstone */ 
  hash_table->hashes[i] = 1; 
 
  /* Be GC friendly */ 
  hash_table->keys[i] = NULL; 
  hash_table->values[i] = NULL; 
 
  hash_table->nnodes--; 
} 


In this code, the destroy callbacks may modify the layout of the hashtable (a shrink for example), and hence i would be outdated.

The correct way to handle this is to do all modifications to the table and then call the callback functions.

The case for finish() is less well-defined.  In cairo and harfbuzz, the way we implement it is while (not empty) remove one item.  Another approach would be: while not empty, steal the entire data arrays and then free them.
Comment 1 Behdad Esfahbod 2011-05-18 05:38:37 UTC
Note: in both harfbuzz and cairo we also have to deal with locks.  Implementing a locked hashtable is not as easy of sandwiching all calls with lock/unlock pairs, because of the destroy notifies.  Ryan correctly points out that a recursive-lock would work.   Alternatively, one has to lock, do all modifications, unlock, and call destroy notifies.  But we don't have any locks to worry about here...
Comment 2 Matthias Clasen 2011-05-18 11:19:37 UTC
I agree that it would be good to make destroy notifies safe for modifying the table; we've had requests for that before.

Having a test case that exercises this would be a good start
Comment 3 Matthias Clasen 2012-08-28 01:59:50 UTC
This was fixed in commit afc5319a273d2154cb725110f170a7e7c6b87076