[poppler] XRef::fetch mutex lock

Adam Reichold adamreichold at myopera.com
Sat Apr 6 09:03:55 PDT 2013 

Hello,

Am 06.04.2013 18:00, schrieb Adam Reichold:
> 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);
> }

Of course, include the destruction call, i.e.

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);
  pthread_mutexattr_destroy(&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
>>
> _______________________________________________
> poppler mailing list
> poppler at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/poppler
>

More information about the poppler mailing list