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 744012 - Add GMutexLocker
Add GMutexLocker
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gthread
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2015-02-04 20:49 UTC by Xavier Claessens
Modified: 2015-02-09 07:47 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add GMutexLocker (1.81 KB, patch)
2015-02-04 21:42 UTC, Xavier Claessens
none Details | Review
Add GMutexLocker (2.16 KB, patch)
2015-02-05 04:01 UTC, Xavier Claessens
none Details | Review
Add GMutexLocker (2.43 KB, patch)
2015-02-05 15:36 UTC, Xavier Claessens
reviewed Details | Review
Add GMutexLocker (2.54 KB, patch)
2015-02-05 16:56 UTC, Xavier Claessens
committed Details | Review

Description Xavier Claessens 2015-02-04 20:49:09 UTC
We could take advantage of the new g_auto() to have an automatic unlock when returning from a block.

Something inspired from QMutexLocker: https://qt.gitorious.org/qt/qt/source/30aec2948a9bf322c45addb6afd66247572587b8:src/corelib/thread/qmutex.h#L101

Here is a sketchy implementation, but it could do as well the weird pointer trick qt does to avoid using a struct with gboolean?

typedef struct
{
  GMutex *mutex;
  gboolean locked;
} GMutexLocker;

#define G_MUTEX_LOCKER_INIT(mutex) { mutex, FALSE }

static void
g_mutex_locker_lock (GMutexLocker *locker)
{
  g_mutex_lock (locker->mutex);
  locker->locked = TRUE;
}

static void
g_mutex_locker_unlock (GMutexLocker *locker)
{
  g_mutex_unlock (locker->mutex);
  locker->locked = FALSE;
}

static void
g_mutex_locker_clear (GMutexLocker *locker)
{
  if (locker->locked)
    g_mutex_unlock (locker->mutex);
  locker->locked = FALSE;
}

G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC (GMutexLocker, g_mutex_locker_clear)

static void
my_function ()
{
  static GMutex mutex;
  g_auto(GMutexLocker) locker = G_MUTEX_LOCKER_INIT (&mutex);

  /* Stuff that don't need the lock */

  g_mutex_locker_lock (&locker);
  ...
  if (cond)
    /* No need to unlock ! */
    return;
  ...

  /* Optionally early unlock for stuff that don't need the lock */
  g_mutex_locker_unlock(&locker);
  ...
}
Comment 1 Colin Walters 2015-02-04 20:54:59 UTC
I use this style in my code that uses cleanup functions; basically
anonymous inner scopes:

static void
my_function()
{
  static GMutex mutex;

  /* Stuff that don't need the lock */

  { g_auto(GMutexLocker) locker = G_MUTEX_LOCKER_INIT (&mutex);
    /* Note new anonymous scope */

    if (cond) {
      /* No need to unlock ! */
      return;
    }
  }

  /* Now the mutex is unlocked here too */
}
Comment 2 Xavier Claessens 2015-02-04 21:42:54 UTC
Created attachment 296154 [details] [review]
Add GMutexLocker
Comment 3 Xavier Claessens 2015-02-04 21:46:06 UTC
Wanted to play with the idea, basically just copied QMutexLocker.

@Colin: you mean you would use my proposed GMutexLocker or your code snippet is wrong and you have another way of doing it?
Comment 4 Xavier Claessens 2015-02-04 21:54:08 UTC
Example how to use my proposed API:

{
  static GMutex mutex;
  g_auto(GMutexLocker) locker = g_mutex_locker_init (&mutex);

  /* Here you have the lock */

  ...
  if (cond)
    /* No need to unlock ! */
    return;
  ...

  g_mutex_locker_unlock (&locker);

  /* Here you don't have the lock */
}
Comment 5 Colin Walters 2015-02-04 22:13:26 UTC
I meant to demonstrate with my code example that one can define a variant of this cleanup function that does not need an additional "locked" boolean.
Comment 6 Xavier Claessens 2015-02-04 22:24:48 UTC
Ok, I see, can indeed be stripped down to this if we don't want to let unlock/relock:

typedef GMutex* GMutexLocker;

static inline GMutexLocker
g_mutex_locker_init (GMutex *mutex)
{
  g_mutex_lock (mutex);
  return mutex;
}

static inline void
g_mutex_locker_clear (GMutexLocker *locker)
{
  g_mutex_unlock (*locker);
}

G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(GMutexLocker, g_mutex_locker_clear)
Comment 7 Xavier Claessens 2015-02-05 04:01:37 UTC
Created attachment 296167 [details] [review]
Add GMutexLocker
Comment 8 Allison Karlitskaya (desrt) 2015-02-05 08:57:42 UTC
Lars and I chatted about this during FOSDEM as well and here are a few comments about the proposed approach.

We should either have GMutexLocker be a proper type or not.  If it is a proper type then it ought to be unconditionally defined, with a normal _init and _clear function.  The other alternative is that we stop pretending and just define a special syntax specifically for locks.

When discussing it, I think we came to the conclusion that a pointer-based API would be nicer (and also theorised about the name GMutexHolder).

GMutexHolder *
g_mutex_holder_new (GMutex *mutex)
{
  g_mutex_lock (mutex);
  return (GMutexHolder *) mutex;
}

void
g_mutex_holder_free (GMutexHolder *holder)
{
  g_mutex_unlock ((GMutex *) holder);
}


this way we could use g_autoptr():

  g_autoptr(GMutexHolder) *lock = g_mutex_holder_new (&lock);

without relying on either

  a) a non-standard return-struct-by-value-but-really-is-actually-a-pointer _init function; or

  b) a separate _init line


I'd really like to hear input from the Vala developers about which approach they think would better suit use from that language.
Comment 9 Xavier Claessens 2015-02-05 12:23:15 UTC
I came to the same conclusion, that's exactly my last patch, with différent names.
Comment 10 Allison Karlitskaya (desrt) 2015-02-05 12:32:54 UTC
(In reply to comment #9)
> I came to the same conclusion, that's exactly my last patch, with différent
> names.

It's not, and I talked about the reasons why:

 - you only define it on GNUC

 - the non-standard _init function is weird

 - the lack of a proper _clear function is also weird

 - g_auto() vs. g_autoptr() and _init/_clear vs. _new/_free


These may seem like some pretty pedantic things to argue about, but conventions are very important in GLib.
Comment 11 Xavier Claessens 2015-02-05 15:18:26 UTC
(In reply to comment #10)
>  - you only define it on GNUC

I don't see a usecase for it without g_autoptr() which is not defined outside of GNUC, afaik. But as you wish, doesn't change anything for me.

>  - the non-standard _init function is weird

Included that as "with different names". I agree new/free is better here.

>  - the lack of a proper _clear function is also weird

At first I though it would be pointless since the whole point is to clear with g_autoptr magic. But you're right it's better to have complete API, and it would also allow nice early-unlock with g_clear_pointer(&locker, g_mutex_locker_free); and relock with locker = g_mutex_locker_new(&muter);

>  - g_auto() vs. g_autoptr() and _init/_clear vs. _new/_free

My proposed patch, in contrary to what I said before, does use g_autoptr().
Comment 12 Xavier Claessens 2015-02-05 15:36:15 UTC
Created attachment 296201 [details] [review]
Add GMutexLocker
Comment 13 Xavier Claessens 2015-02-05 15:38:06 UTC
I just have one doubt: _free() funcs are usually NULL-safe. Should it be changed to: if(locker)g_mutex_unlock((GMutex*)locker); or rename it to _clear(), or it's fine line that?
Comment 14 Allison Karlitskaya (desrt) 2015-02-05 15:41:08 UTC
Review of attachment 296201 [details] [review]:

Looks nice, modulo the name (but I don't care _that_ much) and some minor docs issues.

I do still want to wait to hear that this is nicely usable from Vala, however.

::: glib/gthread.h
@@ +281,3 @@
+ * Lock @mutex and return a new #GMutexLocker. Unlock with
+ * g_mutex_locker_free(). Using g_mutex_lock() or g_mutex_unlock() on @mutex
+ * while a #GMutexLocker exists leads to undefined behaviour.

This is not technically true.  g_mutex_lock() [from another thread] on a mutex with an existing GMutexLocker is perfectly well defined: it blocks until the lock is released.

@@ +286,3 @@
+ * |[
+ * {
+ *   static GMutex mutex;

move the mutex definition out of the function, perhaps?

this is the more usual case...
Comment 15 Allison Karlitskaya (desrt) 2015-02-05 15:54:56 UTC
(In reply to comment #13)
> I just have one doubt: _free() funcs are usually NULL-safe. Should it be
> changed to: if(locker)g_mutex_unlock((GMutex*)locker); or rename it to
> _clear(), or it's fine line that?

_free functions are not usually NULL safe.
Comment 16 Xavier Claessens 2015-02-05 16:06:02 UTC
(In reply to comment #14)
> Review of attachment 296201 [details] [review]:
> 
> Looks nice, modulo the name (but I don't care _that_ much) and some minor docs
> issues.

I don't care about locker/holder neither. I used locker because I've seen that in Qt API.

> I do still want to wait to hear that this is nicely usable from Vala, however.

Ok

> ::: glib/gthread.h
> @@ +281,3 @@
> + * Lock @mutex and return a new #GMutexLocker. Unlock with
> + * g_mutex_locker_free(). Using g_mutex_lock() or g_mutex_unlock() on @mutex
> + * while a #GMutexLocker exists leads to undefined behaviour.
> 
> This is not technically true.  g_mutex_lock() [from another thread] on a mutex
> with an existing GMutexLocker is perfectly well defined: it blocks until the
> lock is released.

Right, it's only doing g_mutex_unlock(&mutex) that can lead to undefined behaviour when the auto cleanup will call g_mutex_locker_free(). Just wanted to state that it's better not to play with the mutex behind locker's back within the same thread.

> @@ +286,3 @@
> + * |[
> + * {
> + *   static GMutex mutex;
> 
> move the mutex definition out of the function, perhaps?
> 
> this is the more usual case...

ok
Comment 17 Luca Bruno 2015-02-05 16:11:14 UTC
At a first glance it should be clean to use in vala. It would behave like any
other non-refcount object created (i.e. a Compact class), and which vala will
free automatically.
We won't use g_auto of course.

Talking about free(), what about using a refcount instead?
Comment 18 Xavier Claessens 2015-02-05 16:56:52 UTC
Created attachment 296208 [details] [review]
Add GMutexLocker
Comment 19 Olivier Crête 2015-02-05 17:44:16 UTC
Can we have a macro like this instead of having to do the whole g_autoptr assignment dance every time?

#define G_LOCK_FOR_SCOPE(mutex) g_autoptr(GMutexLocker) G_PASTE(mutex,locker) = g_mutex_locker_new (mutex);
Comment 20 Christian Hergert 2015-02-05 17:57:20 UTC
How about just adding a method on the various synchronization primitives that return a pointer instead of gboolean? Then we can just define the release function to unlock the mutex. No new structs, etc.

{
  g_autoptr(GMutex) mutex = g_mutex_acquire (&my_mutex);
}
Comment 21 Xavier Claessens 2015-02-05 17:58:36 UTC
G_PASTE(mutex,locker) won't work if mutex is "self->mutex". But some kind of macro to make it shorted could be nice, I agree.

What about:

#define G_DECLARE_LOCKER(name, mutex) g_autoptr(GMenuLocker) name = g_mutex_locker_new(mutex);
Comment 22 Xavier Claessens 2015-02-05 18:01:51 UTC
(In reply to comment #20)
> How about just adding a method on the various synchronization primitives that
> return a pointer instead of gboolean? Then we can just define the release
> function to unlock the mutex. No new structs, etc.
> 
> {
>   g_autoptr(GMutex) mutex = g_mutex_acquire (&my_mutex);
> }

In the end, that's exactly what the proposed patch does, but with different naming. IMO having a specific typedef make it more explicit.
Comment 23 Xavier Claessens 2015-02-05 19:48:22 UTC
Code pushed as wip/xclaesse/locker
Comment 24 Allison Karlitskaya (desrt) 2015-02-06 11:04:30 UTC
(In reply to comment #20)
> How about just adding a method on the various synchronization primitives that
> return a pointer instead of gboolean? Then we can just define the release
> function to unlock the mutex. No new structs, etc.
> 
> {
>   g_autoptr(GMutex) mutex = g_mutex_acquire (&my_mutex);
> }

I don't care for this because I consider it to be extremely surprising that the cleanup function for GMutex would be _unlock and not _free.
Comment 25 Allison Karlitskaya (desrt) 2015-02-06 11:12:48 UTC
Attachment 296208 [details] pushed as 1404d3e - Add GMutexLocker

I pushed your patch with two minor changes:

 - moved the autoptr define to the separate header

 - added a warning about portability to the docs
Comment 26 Fan, Chun-wei 2015-02-09 07:47:36 UTC
Hi,

I have opened up Bug 744190 as the " Add GMutexLocker" patch broke builds on compilers which did not have "inline" in C-mode.

With blessings, thank you!