[poppler] XRef::fetch mutex lock

Adam Reichold adamreichold at myopera.com
Sat Apr 6 09:00:10 PDT 2013 

Hello,

Am 06.04.2013 17:45, schrieb Adam Reichold:
> Hello again,
> 
> Am 06.04.2013 17:40, schrieb Adam Reichold:
>> Hello,
>>
>> Am 06.04.2013 17:05, schrieb Albert Astals Cid:
>>> El Divendres, 5 d'abril de 2013, a les 17:30:49, Adam Reichold va escriure:
>>>> Hello,
>>>>
>>>> Am 05.04.2013 00:38, schrieb Albert Astals Cid:
>>>>> El Dimecres, 3 d'abril de 2013, a les 19:32:23, Albert Astals Cid va 
>>> escriure:
>>>>>> El Dimecres, 3 d'abril de 2013, a les 08:00:34, Thomas Freitag va 
>>> escriure:
>>>>>>> Am 03.04.2013 00:01, schrieb Albert Astals Cid:
>>>>>>>> El Dimarts, 2 d'abril de 2013, a les 10:36:53, Thomas Freitag va
>>>>>
>>>>> escriure:
>>>>>>>>> Am 02.04.2013 09:22, schrieb Thomas Freitag:
>>>>>>>>>> Hi Albert!
>>>>>>>>>>
>>>>>>>>>> Am 27.03.2013 12:29, schrieb Thomas Freitag:
>>>>>>>>>>> Sorry, pushed the wrong button, here my answer to the list:
>>>>>>>>>>>
>>>>>>>>>>> Am 27.03.2013 11:41, schrieb Albert Astals Cid:
>>>>>>>>>>>> Why do we pass around the recursion integer around?
>>>>>>>>>>>
>>>>>>>>>>> No idea: The recursive integer wasn't introduced by the thread safe
>>>>>>>>>>> patch, it was already there. I just used it and extend some
>>>>>>>>>>> functions
>>>>>>>>>>> which incorrectly ignore that parameter instead of passing it to the
>>>>>>>>>>> called functions. Probably I missed some more than only the one in
>>>>>>>>>>> DCTStream::DCTStream.
>>>>>>>>>>
>>>>>>>>>> I just ran a regression test over my PDF files, but I didn't get any
>>>>>>>>>> dead locks. So if You want me to solve that problem, You should
>>>>>>>>>> provide me that PDF. (Regardless if You prefer to use recursive
>>>>>>>>>> mutex:
>>>>>>>>>> the endless loop detection need to pass the recursive integer
>>>>>>>>>> everywhere!)
>>>>>>>>>
>>>>>>>>> Attached a patch which corrects it. I made it in my sleep, 'cause the
>>>>>>>>> dead lock was probably caused by my patch for bug 61994 (even if that
>>>>>>>>> PDF doesn't cause any dead locks). This time I also respect the
>>>>>>>>> DCTStream usage if LIBJPEG is disabled :-)
>>>>>>>>
>>>>>>>> Sure, this fixes the issue (and we should commit it to help with the
>>>>>>>> infinite recursion issue), but what's your opinion on making our
>>>>>>>> mutexes
>>>>>>>> recursive?
>>>>>>>
>>>>>>> I think, a quite good discussion about recursive and non recursive mutes
>>>>>>> was here:
>>>>>>> http://stackoverflow.com/questions/187761/recursive-lock-mutex-vs-non-re
>>>>>>> cu
>>>>>>> rs ive-lock-mutex. This convinced me to vote for a usage of recursive
>>>>>>> mutex in poppler: 1. xpdf and therfore poppler wasn't really designed
>>>>>>> for
>>>>>>> the usage of threads. We have it now working, but if we cherish on the
>>>>>>> usage of non recursive mutex we will always run into dead lock problems
>>>>>>> with futur patches again, so testing patches will become more expensive.
>>>>>>> 2. Since poppler nowhere uses emergency exits with "throw", we don't
>>>>>>> have the problem that we have to keep track that a thread releases a
>>>>>>> mutex so often it aquires it.
>>>>>>> 3. The only (small) problem I see is the sentence "Not all systems
>>>>>>> supporting pthreads also support recursive mutexes, but if they want to
>>>>>>> be POSIX conform, they have to
>>>>>>> <http://www.opengroup.org/onlinepubs/009695399/functions/pthread_mutexat
>>>>>>> tr
>>>>>>> _g ettype.html>."
>>>>>>
>>>>>> To be honest i don't think we should care much about the minority of
>>>>>> systems there that don't support recursive mutexes.
>>>>>
>>>>> Here's a quick patch I've hacked together (still needs some work in the
>>>>> buildsystem side).
>>>>
>>>> Not something that breaks either, but maybe a performance improvement:
>>>> According to the manual page of 'phtread_mutexattr_init', a single mutex
>>>> attribute object can be used to initialize several mutexes, hence we
>>>> possibly don't need to keep it in the GooLock structure and could use
>>>> just a single instance that is initialized once. Not sure whether this
>>>> really hurts with the number of mutexes we have.
>>>
>>> I tried doing that but then i needed a "global" mutex attribute that i needed 
>>> to initialize, and that meant i needed to include a mutex to protect the 
>>> initialization of the attribute or "force" all the mutexes to be created in 
>>> the same thread. At the end i went the "easy" way of having one mutex 
>>> attribute per mutex, it's not like we are going to use that much more memory 
>>> and it's just easier to understand codewise.
>>
>> I see and I agree that it is probably simpler that way. But then I think
>> there should be a call to 'pthread_mutexattr_destroy' in
>> 'gDestroyMutex', shouldn't there?
> 
> Sorry, just noticed that you already included it in the commit...

Again I am sorry (must be the weekend), but after reading the POSIX
programmer's manual page again, I think the 'mutexattr' object does not
need to survive the mutex itself, e.g.

typedef pthread_mutex_t GooMutex;

inline void gInitMutex(GooMutex *m) {
  pthread_mutexattr_t mutexattr;
  pthread_mutexattr_init(&mutexattr);
  pthread_mutexattr_settype(&mutexattr, PTHREAD_MUTEX_RECURSIVE);
  pthread_mutex_init(&m, &mutexattr);
}

should work as well.

Best regards, Adam.

P.S.: At least this is how I would interpret

After a mutex attributes object has been used to initialize one or more
mutexes,  any  function  affecting  the  attributes  object  (including
destruction) shall not affect any previously initialized mutexes.

from the manual page.

>>> Cheers,
>>>   Albert
>>>
>>> P.S: I had to add some -lpthread to the cmake buildsystem but not to the 
>>> automake one, find it a bit weird to be honest but it's what i needed to make 
>>> it compile in both.
>>
>> Yes, sounds strange. Maybe the 'ACX_PTHREAD' macro has some side effects
>> w.r.t. that?
>>
>> Best regards, Adam.
>>
>>>>
>>>> Best regards, Adam.
>>>>
>>>>> I'll finish it properly tomorrow but meanwhile can you guys check if it
>>>>> breaks something related to the multithreading side?
>>>>>
>>>>> I haven't touched the windows side since somewhere i've read that
>>>>> CriticalSections are already recursive. Can anyone confirm?
>>>>>
>>>>> Cheers,
>>>>>
>>>>>   Albert
>>>>>>
>>>>>> Cheers,
>>>>>>
>>>>>>   Albert
>>>>>>>
>>>>>>> Cheers,
>>>>>>> Thomas
>>>>>>>
>>>>>>>> It should help us making the code simpler since we could just drop all
>>>>>>>> the
>>>>>>>> DoNotLockMutex cases, no?
>>>>>>>>
>>>>>>>> Cheers,
>>>>>>>>
>>>>>>>>    Albert
>>>>>>>>>
>>>>>>>>> Cheers,
>>>>>>>>> Thomas
>>>>>>>>>
>>>>>>>>>> Cheers,
>>>>>>>>>> Thomas
>>>>>>>>>>
>>>>>>>>>>>> Wouldn't a real recursive mutex be enough?
>>>>>>>>>>>
>>>>>>>>>>> No idea: I used in the thread safe patch just the pthread_lock which
>>>>>>>>>>> was defined. I'm not knowing what happens when we change it globally
>>>>>>>>>>> i.e. if it affects other usages of the GooMutex. Just feel free to
>>>>>>>>>>> change it, You should have the better overview of usages than I
>>>>>>>>>>> have.
>>>>>>>>>>>
>>>>>>>>>>> Cheers,
>>>>>>>>>>> Thomas
>>>>>>>>>>>
>>>>>>>>>>>> I'm asking because i have a document here (that sadly i can't
>>>>>>>>>>>> share)
>>>>>>>>>>>> that is deadlocking
>>>>>>>>>>>> itself there because we are not passing the recursion integer
>>>>>>>>>>>> everywhere (we lose it in DCTStream::DCTStream)
>>>>>>>>>>>> as shown in this backtrace
>>>>>>>>>>>>
>>>>>>>>>>>> #0  __lll_lock_wait () at
>>>>>>>>>>>> ../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
>>>>>>>>>>>> #1  0x00007ffff599217c in _L_lock_982 () from
>>>>>>>>>>>> /lib/x86_64-linux-gnu/libpthread.so.0
>>>>>>>>>>>> #2  0x00007ffff5991fcb in __GI___pthread_mutex_lock
>>>>>>>>>>>> (mutex=0x71dc10)
>>>>>>>>>>>> at pthread_mutex_lock.c:64
>>>>>>>>>>>> #3  0x00007ffff5f02f73 in MutexLocker::MutexLocker
>>>>>>>>>>>> (this=0x7fffffffcfb0, mutexA=0x71dc10, modeA=DoLockMutex) at
>>>>>>>>>>>> /home/tsdgeos/devel/poppler/goo/GooMutex.h:72
>>>>>>>>>>>> #4  0x00007ffff5fac8e2 in XRef::fetch (this=0x71db50, num=5, gen=0,
>>>>>>>>>>>> obj=0x7fffffffd0b0, recursion=0) at
>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/XRef.cc:1137
>>>>>>>>>>>> #5  0x00007ffff5f8a013 in Object::fetch (this=0x71fe08,
>>>>>>>>>>>> xref=0x71db50, obj=0x7fffffffd0b0, recursion=0) at
>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/Object.cc:122
>>>>>>>>>>>> #6  0x00007ffff5f19de4 in Dict::lookup (this=0x71fb60,
>>>>>>>>>>>> key=0x7ffff606fbba "Height", obj=0x7fffffffd0b0, recursion=0) at
>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/Dict.cc:256
>>>>>>>>>>>> #7  0x00007ffff5f032e9 in Object::dictLookup (this=0x7fffffffd5a0,
>>>>>>>>>>>> key=0x7ffff606fbba "Height", obj=0x7fffffffd0b0, recursion=0) at
>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/Object.h:315
>>>>>>>>>>>> #8  0x00007ffff601f8a3 in DCTStream::DCTStream (this=0x7832b0,
>>>>>>>>>>>> strA=0x720780, colorXformA=-1, dict=0x7fffffffd5a0) at
>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/DCTStream.cc:72
>>>>>>>>>>>> #9  0x00007ffff5f9bf6d in Stream::makeFilter (this=0x720780,
>>>>>>>>>>>> name=0x71fef0 "DCTDecode", str=0x720780, params=0x7fffffffd1e0,
>>>>>>>>>>>> recursion=1, dict=0x7fffffffd5a0) at
>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/Stream.cc:311
>>>>>>>>>>>> #10 0x00007ffff5f9b71a in Stream::addFilters (this=0x720780,
>>>>>>>>>>>> dict=0x7fffffffd5a0, recursion=1) at
>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/Stream.cc:184
>>>>>>>>>>>> #11 0x00007ffff5f9204b in Parser::makeStream (this=0x71fcd0,
>>>>>>>>>>>> dict=0x7fffffffd5a0, fileKey=0x0, encAlgorithm=cryptRC4,
>>>>>>>>>>>> keyLength=1146103040, objNum=3, objGen=0, recursion=1,
>>>>>>>>>>>> strict=false)
>>>>>>>>>>>> at /home/tsdgeos/devel/poppler/poppler/Parser.cc:280
>>>>>>>>>>>> #12 0x00007ffff5f918e6 in Parser::getObj (this=0x71fcd0,
>>>>>>>>>>>> obj=0x7fffffffd5a0, simpleOnly=false, fileKey=0x0,
>>>>>>>>>>>> encAlgorithm=cryptRC4, keyLength=1146103040, objNum=3, objGen=0,
>>>>>>>>>>>> recursion=0, strict=false) at
>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/Parser.cc:131
>>>>>>>>>>>> #13 0x00007ffff5facdd3 in XRef::fetch (this=0x71db50, num=3, gen=0,
>>>>>>>>>>>> obj=0x7fffffffd5a0, recursion=0) at
>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/XRef.cc:1197
>>>>>>>>>>>> #14 0x00007ffff5f8a013 in Object::fetch (this=0x71e128,
>>>>>>>>>>>> xref=0x71db50, obj=0x7fffffffd5a0, recursion=0) at
>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/Object.cc:122
>>>>>>>>>>>> #15 0x00007ffff5f19de4 in Dict::lookup (this=0x71e660, key=0x7206e0
>>>>>>>>>>>> "Im1", obj=0x7fffffffd5a0, recursion=0) at
>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/Dict.cc:256
>>>>>>>>>>>> #16 0x00007ffff5f032e9 in Object::dictLookup (this=0x71e598,
>>>>>>>>>>>> key=0x7206e0 "Im1", obj=0x7fffffffd5a0, recursion=0) at
>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/Object.h:315
>>>>>>>>>>>> #17 0x00007ffff5f2a647 in GfxResources::lookupXObject
>>>>>>>>>>>> (this=0x71e590, name=0x7206e0 "Im1", obj=0x7fffffffd5a0) at
>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/Gfx.cc:411
>>>>>>>>>>>> #18 0x00007ffff5f3f41e in Gfx::opXObject (this=0x71e440,
>>>>>>>>>>>> args=0x7fffffffd720, numArgs=1) at
>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/Gfx.cc:4114
>>>>>>>>>>>> #19 0x00007ffff5f2bfd6 in Gfx::execOp (this=0x71e440,
>>>>>>>>>>>> cmd=0x7fffffffd6e0, args=0x7fffffffd720, numArgs=1) at
>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/Gfx.cc:858
>>>>>>>>>>>> #20 0x00007ffff5f2b82b in Gfx::go (this=0x71e440, topLevel=true) at
>>>>>>>>>>>> /home/tsdgeos/devel/poppler/poppler/Gfx.cc:717
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Cheers,
>>>>>>>>>>>>
>>>>>>>>>>>>     Albert
>>>>>>>>>>>>
>>>>>>>>>>>> _______________________________________________
>>>>>>>>>>>> poppler mailing list
>>>>>>>>>>>> poppler at lists.freedesktop.org
>>>>>>>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
>>>>>>>>>>>>
>>>>>>>>>>>> .
>>>>>>>>>>>
>>>>>>>>>>> _______________________________________________
>>>>>>>>>>> poppler mailing list
>>>>>>>>>>> poppler at lists.freedesktop.org
>>>>>>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
>>>>>>>>>>>
>>>>>>>>>>> .
>>>>>>>>>>
>>>>>>>>>> _______________________________________________
>>>>>>>>>> poppler mailing list
>>>>>>>>>> poppler at lists.freedesktop.org
>>>>>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
>>>>>>>>>>
>>>>>>>>>> .
>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> poppler mailing list
>>>>>>>> poppler at lists.freedesktop.org
>>>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
>>>>>>>>
>>>>>>>> .
>>>>>>
>>>>>> _______________________________________________
>>>>>> poppler mailing list
>>>>>> poppler at lists.freedesktop.org
>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> poppler mailing list
>>>>>> poppler at lists.freedesktop.org
>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
>>>>
>>>> _______________________________________________
>>>> poppler mailing list
>>>> poppler at lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
>>> _______________________________________________
>>> poppler mailing list
>>> poppler at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/poppler
>>>
>> _______________________________________________
>> poppler mailing list
>> poppler at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/poppler
>>
> _______________________________________________
> poppler mailing list
> poppler at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/poppler
>

More information about the poppler mailing list