thumbnail.cc uses Glib::Mutex recusively

[Beep6581]

Originally reported on Google Code with ID 1684

thumbnail.cc uses Glib::Mutex recusively, for example getThumbnailSize() locks the mutex,
and then calls  getProcParams() which itself locks the mutex.

The Glibmm docs say that this should not work:
"Glib::Mutex is not recursive, i.e. a thread will deadlock, if it already has locked
the mutex while calling lock(). Use Glib::RecMutex instead, if you need recursive mutexes."

It seems that while in the current version of glibmm on windows it works without a
deadlock, it does deadlock on linux (hence it being ifdef'ed out).

It would be wise to follow the advice of the docs, and switch the code to use RecMutex
(if recursiveness is needed). The attached patch does this. It works for me on linux,
but i have not tested on windows.

This was noticed while investigating issue 1675, but i think is really a separate issue.
There a plenty of other bits of code using Glib::Mutex, i have not checked those yet.

Click on Preferences > About > Version and paste all that here.
current hg b681d69af34d

Reported by samtygier on 2013-01-16 23:03:31

---

• _Attachment: swtich_to_recmutex.diff_

[Beep6581]

I'll test it tonight, but i'm a bit interrogative on the reason why Linux doesn't need
mutex? Could you provide a patch with ifdefs removed, so Lunix users can test both
versions?

Reported by natureh.510 on 2013-01-17 13:59:21

• Status changed: Accepted
• Labels added: Priority-High
• Labels removed: Priority-Low

[Beep6581]

Compilés and run fines on Win7,  as much as before (for the reason you explained).

Reported by natureh.510 on 2013-01-17 23:34:27

[Beep6581]

the second patch on issue 1659 (attached again here), is the switch to RecMutex and
the removal of the ifdefs. I think it is maybe over cautious in it locking, so i see
some short hangs with it. From comment 8 on 1675 it sounds like things just happen
in different orders on win and linux.

ps: is it better to send patches with a plain 'hg diff', or should i be commiting locally
and using something like 'hg log -r tip -p'. or should i create a public repo somewhere?

Reported by samtygier on 2013-01-18 10:26:48

---

• _Attachment: fix_1675_2.patch_

[Beep6581]

IMHO, a plain 'hg diff' is sufficient, and very flexible when you have to revert thing
at some point. Mercurial for Eclipse helps a lot!

Reported by natureh.510 on 2013-01-18 10:45:13

[Beep6581]

This issue was closed by revision b4aa66ef5205.

Reported by rinni@gmx.net on 2013-01-20 11:46:50

• Status changed: Fixed

[Beep6581]

How is this issue related to  revision b4aa66ef5205?

Reported by michaelezra000 on 2013-01-20 15:50:15

[Beep6581]

Sorry, I wanted to close #1685 :(

Reported by rinni@gmx.net on 2013-01-20 18:27:31

• Status changed: Accepted

[Beep6581]

:)

Reported by michaelezra000 on 2013-01-20 20:07:56

[Beep6581]

What's the status here? Patch was submitted, can it be committed?

Reported by entertheyoni on 2013-02-19 22:41:34

• Status changed: PatchSubmitted

[Beep6581]

Bump
samtygier, is this still relevant? Do you want to commit or close?
Patch from issue 1659 was committed 3 weeks ago.

Reported by entertheyoni on 2013-03-07 01:43:56

[Beep6581]

Here is a patch that introduce a self made mutex class, called MyMutex, with it's MyMutex::MyLock
subclass.

It can have 2 behavior, depending on a build flag (driven by a CMake parameter of the
same name), called STRICT_MUTEX.

If set to true, the MyMutex will behave like POSIX mutex on all platform, i.e. will
deadlock on the second call (in fact, MyMutex will make the application crash, going
back to the debugger in the Debug build). This is the default behavior.

If set to false, the MyMutex will behave like RecMutex on all platform. I don't know
of this is something one may want, as explained on the web. Anyway, it's possible,
but under your responsibility.

I've converted almost all Mutex to MyMutex, excepted one critical one in rtgui/thumbimageupdater.cc,
which uses the Glib::Threads::Cond with it.

I've had an hard time to make the application return in the debugger on the windows
platform, with a usable stack trace. There isn't much stacks there, but there are more
on linux platforms.

I've then decided to remove the #ifndef WIN32 conditions around mutex. I had to solve
2 or 3 deadlocks then, but it was pretty easy with this new mechanism. I don't know
if all deadlock are removed, so we should test it thoroughly before committing.

Reported by natureh.510 on 2013-08-04 03:05:02

• Labels added: Stability

---

• Attachment: NonRecursiveMutex.patch

[Beep6581]

Hi Hombre, back from vacancies I'll test this Issue tomorrow! 

Ingo

Reported by heckflosse@i-weyrich.de on 2013-08-05 23:46:34

[Beep6581]

I'm on holiday and wished to code as much as possible before going traveling, so I finally
committed this patch, I've tested it under Win7/64 and OpenSuse Linux 12.3.

Reported by natureh.510 on 2013-08-11 21:36:42

• Status changed: FixedPendingConfirmation

[Beep6581]

PS: see comment #11 on how to use the new MyMutex and MyMutex::MyLock class, and the
STRICT_MUTEX define.

Reported by natureh.510 on 2013-08-11 21:39:32

[Beep6581]

I used your patch in default config the last days and didn't detect any abnormalities.
Will test the next days with STRICT_MUTEX set to false.

Ingo

Reported by heckflosse@i-weyrich.de on 2013-08-11 21:45:55

[Beep6581]

In fact, I'm wondering if i should remove that switch and use the strict mode as default.
I thought that the permissive mode would be interesting in release builds, to avoid
unwanted deadlock. It should be pretty safe given that C++ handle the release automagicaly
in the destructor (provided that devs are careful when calling the locking and unlocking
method directly), but that's subject to discussion ;)

The advantage of those new classes is that they'll behave the same on all platforms.

Reported by natureh.510 on 2013-08-11 22:19:17

[Beep6581]

Maybe the deadlock happening in issue 882 is related to this? I haven't looked at 894
closely but did a quick backtrace and noted that a thread was stuck on Thumbnail::decreaseRef()

Reported by torger@ludd.ltu.se on 2013-10-30 13:56:56

[Beep6581]

@Hombre57 what's the status here?

[Hombre57]

IIRC, I saw a change in gcc that would make both Linux and Windows pthread implementation works the same, maybe since gcc 5, but I can't find that information again. We can still close this issue, since the workaround shouldn't introduce any speed penalty. If one want to look in that again, he may simplify the MyMutex classes. However we should keep them for debugging purpose.