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 724743 - Some tweaks re: GRWLock
Some tweaks re: GRWLock
Status: RESOLVED OBSOLETE
Product: glib
Classification: Platform
Component: gthread
2.38.x
Other Linux
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2014-02-19 18:49 UTC by Olivier Brunel (jjacky)
Modified: 2018-05-24 16:18 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
GRWLock: Tweak doc to make things a bit clearer (2.65 KB, patch)
2014-02-19 18:49 UTC, Olivier Brunel (jjacky)
none Details | Review
GRWLock: Tweak re: blocking on g_rw_lock_reader_lock() (1.30 KB, patch)
2014-02-19 18:50 UTC, Olivier Brunel (jjacky)
none Details | Review
GRWLock: add critical warnings for EDEADLK (1.49 KB, patch)
2014-02-19 18:50 UTC, Olivier Brunel (jjacky)
none Details | Review

Description Olivier Brunel (jjacky) 2014-02-19 18:49:11 UTC
As I was using some GRWLock-s I came to realize a few things. First of all, the doc is a bit unclear, e.g. using "any thread" or "any other thread" to refer to the same thing, even though that's not really actually saying the same thing. I'll attach a patch that addresses this, and also makes it clear that trying to lock while already owning a lock is undefined behavior (except for read locks).

This is important especially since one might have expected to get a deadlock, as the doc states it "blocks until" it gets the lock, while in reality it might just return right away without the lock, giving no hints whatsoever that something wrong happened (and will happen again when trying to unlock a lock you don't have).
Speaking of, I'm also adding a patch that adds critical warnings in case of EDEADLK, since that means there's a bug somewhere.

Also, as I was reading the doc about pthreads, I think even with those changes things aren't quite right in GLib, because g_rw_lock_reader_lock() claims to block until it gets the lock, but that's not true: it could fail if the maximum number of read locks has been exceeded.

I wonder if something shouldn't be done as well, e.g. make it return a gboolean, whether the lock has been aquired or not. Otherwise it's a condition which isn't really a bug in the application, yet there's no way to actually know if a read lock is owned or not, which will lead to troubles.

Making it return a gboolean wouldn't break any existing code, but allow future code to make sure a read lock is held, and if not error out or something.
(Note that I have no idea how things work on Window however: could the same be done, or would it then always return TRUE?)
Comment 1 Olivier Brunel (jjacky) 2014-02-19 18:49:51 UTC
Created attachment 269719 [details] [review]
GRWLock: Tweak doc to make things a bit clearer

The doc used different phrasing for the same thing, e.g. "if any thread"
vs "any other thread."

Also make it clear that trying to take a write lock while already having
a lock, or trying to take a read lock while having a write lock, is
undefined.
Comment 2 Olivier Brunel (jjacky) 2014-02-19 18:50:25 UTC
Created attachment 269720 [details] [review]
GRWLock: Tweak re: blocking on g_rw_lock_reader_lock()

There's no guarantee that it will block if another thread is blocking
for a write lock.

See https://bugzilla.gnome.org/show_bug.cgi?id=681980
Comment 3 Olivier Brunel (jjacky) 2014-02-19 18:50:54 UTC
Created attachment 269721 [details] [review]
GRWLock: add critical warnings for EDEADLK

If failing to get a read or write lock due to deadlock condition detected,
i.e. there's a bug somewhere, emit a critical warning.

Can be useful, especially since otherwise there's no way of knowing we don't
actually have a lock, even though the doc says the function either gets
it or waits (deadlock) for it.
Comment 4 GNOME Infrastructure Team 2018-05-24 16:18:36 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/832.