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 778005 - locking issues reported by the ThreadSanitizer
locking issues reported by the ThreadSanitizer
Status: RESOLVED FIXED
Product: folks
Classification: Platform
Component: general
0.11.x
Other Linux
: Normal normal
: Unset
Assigned To: folks-maint
folks-maint
Depends on:
Blocks:
 
 
Reported: 2017-01-31 22:11 UTC by Fabrice Bellet
Modified: 2017-02-01 01:01 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
ThreadSanitizer: unlock of an unlocked mutex (15.32 KB, text/plain)
2017-01-31 22:11 UTC, Fabrice Bellet
Details

Description Fabrice Bellet 2017-01-31 22:11:09 UTC
Created attachment 344670 [details]
ThreadSanitizer: unlock of an unlocked mutex

The ThreadSanitizer plotted several locking issues related to constructs like this :

  public static Debug dup ()
    {
      lock (Debug._instance)
        {
          Debug? _retval = Debug._instance;
          Debug retval;

          if (_retval == null)
            {
              /* use an intermediate variable to force a strong reference */
              retval = new Debug ();
              Debug._instance = retval;
            }
          else
            {
              retval = (!) _retval;
            }

          return retval;
        }
    }

--> The problem is that g_rec_mutex_init() is called on the mutex "_folks_debug_dup_lock_folks_debug__instance" as part of the "retval = new Debug();" statement, the same mutex that is locked/unlocked in the surrounding code block by "lock (Debug._instance)"
Comment 1 Philip Withnall 2017-02-01 00:02:45 UTC
This looks like a Vala bug:
 • Debug._instance is static, so the mutex could be allocated statically and shouldn’t need initialisation.
 • Locking a variable in order to assign to it should be a fairly common idiom, so the lock initialisation behaviour here is a bit weird.
 • In the case I’m misunderstanding how Vala expects the developer to use the `lock` statement here, valac should probably emit an error if it detects a construction of a class within a `lock` block using a lock defined on that class.

In any case, the locking here is pointless, since folks is single-threaded and not thread-safe (and never has been). I’ll remove it.

Would you mind filing a bug against Vala for the problems with the generated locking code?
Comment 2 Philip Withnall 2017-02-01 01:01:03 UTC
Fixed.
Comment 3 Philip Withnall 2017-02-01 01:01:37 UTC
commit 5ae297e5f8e27e75da5f6d483292c6238d47f05e
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Wed Feb 1 00:14:07 2017 +0000

    core: Remove locking everywhere
    
    folks is single-threaded, and not thread-safe. It never has been. The
    locking was added ‘because it won’t hurt’ — but it actually generates
    invalid code which double-unlocks mutexes. This doesn’t hurt (in that it
    doesn’t crash), but it certainly isn’t valid, and (rightfully) generates
    a lot of ThreadSanitizer noise.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=778005