Exiv2

It reproes only in Release build for me so I can't debug it.

digiKam use multithreading to do exactly the same task than you, and it do not crash. We use also Qt as i'm sure that you know already as i i see my comments in your code.

Your trace show a XMP problem. 99% of chance that XMP SDK from Adobe is not initiate properly at your application startup...

Gilles Caulier

Gilles Caulier wrote:

Your trace show a XMP problem. 99% of chance that XMP SDK from Adobe is not initiate properly at your application startup...

Thanks for the fast response!

Umm. Can you please tell me how to initiate it "properly"?

Robin Mills wrote:

I made a change last September in lookupNsRegistry() and I'm wondering if this is thread-safe. I'm on vacation at the moment and my time to investigate is limited. Are you using the shipping version of v0.25 which does not have my change to lookupNsRegistry().

I'm using "latest stable" I was able to find which is v0.25 atm.

A have added a call to Exiv2::XmpParser::initialize() in the beginning of my app's startup and it still crashes inside lookupNsRegistry().

Overall I didn't see how 0.26 is going, but static variables and threading is a sure recipe for disaster.

BTW, is trunk in SVN "production ready"? Or there're is some big testing planned?

This issue has nothing to do with initialize() which is racy as well (if you don't call it in the beginning of the startup).

The problem is that XmpProperties::registerNs(), XmpProperties::lookupNsRegistry() and XmpProperties::unregisterNs()
have pretty obvious race conditions accessing std::map<std::string, XmpNsInfo> _nsRegistry.

All of them should have locking/etc. Code in trunk (is it future 0.26) is same as 0.25 for the methods above.

I've created a patch with a fix and tested it running ~150 threads in parallel. It crashes if I use old lib and does not if I use lib with my patch. I've added RW lock for operations with NsRegistry nsRegistry_

So the patch for:

- properties.cpp
https://github.com/Ribtoks/xpiks/blob/db25d22f7df71ba3b18bbf5e57a2d94658ddf44b/src/exiv2-0.25/rwlock.patch

- properties.h
is topmost here
https://github.com/Ribtoks/xpiks/commit/db25d22f7df71ba3b18bbf5e57a2d94658ddf44b

- new file rwlock.hpp
https://github.com/Ribtoks/xpiks/blob/77e356af7b02a5a6e9aecb4c1f2966a79c618d79/src/exiv2-0.25/include/exiv2/rwlock.hpp

I know every project has it's own rules for submitting patches and stuff, but I'm really super busy right now to read them and read docs about SVN how to submit my patch over there
(it would be so much easier if you guys just used GitHub or Bitbucket - just look how many clones of Exiv2 project exist nowadays over there).

Gilles Caulier wrote:

digiKam use multithreading to do exactly the same task than you, and it do not crash.
Gilles Caulier

It does not mean that it's stable, but means nobody found a race condition there.

If you will look in the code, multithreaded access to NsRegistry (properties.cpp) can lead to race condition: a reader who cycles over the std::map in lookupNsRegistry() and writer who calls registerNs() which modifies the very same std::map at the same time.

All the changes in patch summarized (+ fixes for Windows build)
https://bitbucket.org/ribtoks/xpiks-deps/commits/18181953b628ed56719fd8a8c1774eeac3a0999b?at=master

I've added issue #1188 to support C++11.

You code is still failing to compile (as I found after I'd tried to build the patch manually). I've added the missing file rwlock.h, however there's a static rowlock_ missing from somewhere.

Still doesn't compile:

libtool: compile:  g++ -ggdb -fvisibility=hidden -fvisibility-inlines-hidden -Wall -Wcast-align \
-Wpointer-arith -Wformat-security -Wmissing-format-attribute -Woverloaded-virtual -W -MMD \
-I../src -I../include/ -I../include/exiv2 -I/usr/local/include -I/usr/local/include \
-DEXV_LOCALEDIR=\"/usr/local/share/locale\" -I../xmpsdk/include -c -DEXV_BUILDING_LIB=1 \
properties.cpp  -fno-common -DPIC -o .libs/properties.o
properties.cpp:2242:46: error: no member named 'rwLock_' in 'Exiv2::XmpProperties'
    Internal::RWLock          XmpProperties::rwLock_;
                              ~~~~~~~~~~~~~~~^
1 error generated.
make[1]: *** [properties.o] Error 1
make: *** [all] Error 2
525 rmills@rmillsmbp:~/gnu/exiv2/bigtiff $ 

Robin Mills wrote:

I've added issue #1188 to support C++11.

You code is still failing to compile (as I found after I'd tried to build the patch manually). I've added the missing file rwlock.h, however there's a static rowlock_ missing from somewhere.

Still doesn't compile:[...]I looked at your new file rwlock.hpp. I think it's OK. We don't have anything in our test suite to test multi-threading, so I can only give it visual inspection. I will review it carefully before I submit it and after it and passes the test suite on all our supported platforms.

Compilation error is because of you didn't apply patch to properties.hpp where the rwLock_ is defined.
Pls take a look at the included file in the previous commit (rwlock_025.svn_patch)

Of course file rwlock.hpp is ok - it's super trivial and it's just wrapper over the library calls. I was talking about refactoring of properties.cpp. It has a lot of tricky methods which call one another - ns(), nsInfo() and friends.

Patch file was generated from differences from 0.25 release, but not from trunk. If this file was modified in trunk, of course there would be merge conflicts. I didn't generate it against trunk because I won't know how to correctly resolve merge conflicts (but you will).

I hope so much that this fix will get into 0.26

A fix for this will go into v0.26. I was too tired last night to deal with this. I've tried your patch on v0.25 and it builds and passes the test suite. I will add it to the trunk and it will ship in v0.26.

I'll review your code. I didn't write Exiv2, so there are parts that I know well, and other dark corners. Everything concerning XMP is in one of the dark places.

I'm not involved in the release process - that is exclusively the domain of the project founder Andreas. He is clever, cautious and careful. I'll be surprised if he has tests beyond the test suite, however I've never discussed it with him.

Fix submitted r4308. Thank You to Tara for the patch.

You have moved my changes well, but you didn't add rwlock.hpp to visual studio projects thus exiv2 will not compile under Windows. I didn't include it to the patch, because it was impossible - I've converted all projects to VS 2015.

Can you please elaborate on what starving you were talking about? If there would be so many readers that writers will never have access? Can you provide an example so I can fix flaws in my patch?

Yes, your test suite should definitely include threading tests.

I would contribute to exiv2 if only I didn't have tons of work on my own project. If I will find any new issues, I will try to fix them in exiv2. Anyway, you can point me out these threading issues so I can investigate whether they will affect me.

Yes, I know "dining philosophers" problem. However, in my understanding starvation is when low-priority thread constantly fails to acquire a resource because of context switches between higher-priority threads. I would not call starvation any situation when some threads are waiting some time in order to acquire the resource (as in this case - readers and writers are supposed to be of equal priority), because such waiting is inevitable part of live with any synchronization.

I completely agree that multithreading is super error-prone and especially for a stranger to a project to refactor some parts of it to multithreaded support. That's why I was asking to review my changes very carefully.

Thanks for the reference to Quantum Framework. Will take a look into that!

There's also another strange thing related to this issue.

Static methods I've mentioned predictably crash in OS X when being used with 30-40 threads, but never crash on Windows 7 and Windows 10 even on different computers. I can't understand why they don't crash.

Sure. Excuse me for distracting you more than necessary. Have a nice time.

https://bitbucket.org/ribtoks/xpiks-deps/commits/23231c5ab8edafd4e873634dae742996ef068e1e

One more fix needed for Windows build to succeed (bool -> BOOLEAN)

Robin, I've just compiled patched exiv2 v0.25 successfully with Visual Studio 2015 and yesterday I did the same with Visual Studio 2013.

https://msdn.microsoft.com/en-us/library/br244843.aspx - SRWLock is a known class. I've included <windows.h> so it should have been resolved. From your description it's not quite clear what can be wrong. I need to investigate the particular case. Maybe there're some other errors which screw SRWLock's definition and VC compiler thinks it's unknown class.

P.S. Me posting here does not oblige you to respond while you're on vacation. I'm just posting important stuff so it won't get lost with time.

Regrettably, SRWLock isn't in VS 2005/8/10/12. Maybe it's possible to use a static CRITICAL_SECTION to achieve our aims. http://stackoverflow.com/questions/3498798/replace-critical-section-with-srw-lock

CRITICAL_SECTION is a Win32 feature, so you should be able to develop/test it with VS2015, even if you decide to use CRITICAL_SECTION in older versions of Visual Studio and SRWLock in 2013/15. There's code in include/exiv2/exv_msvc.h to detect the version of CL in use.

I don't feel obliged to do this on vacation or at any other time! However I like to deal with support issues quickly to let me focus on developing v0.26. I still have several things to finish for v0.26 and I'm in danger of slipping due to support issues consuming my time. I'm going to defer the low priority features of v0.26 to make the schedule. http://dev.exiv2.org/news/2

It's possible to use CriticalSection, but it will be less optimal than Read Write lock. I would prefer to use Read/Write lock whenever is possible for VS 13/15, and use Critical Section in other cases. I will send a patch for that a bit later today.

The current code produces over 100 warnings of type
warning C4800: "BOOLEAN": forcing value to bool 'true' or 'false' (performance warning)
in rwlock.hpp(33) and rwlock.hpp(43) with MSVC2015.

Also function TryAcquireSRWLockExclusive is only available in Windows 7 and later. So the code will not run on Windows XP and Vista.

Pls see comment #25 - I've added one more tiny patch. dev.exiv2.org/issues/1187#note-25

TryAcquireSRWLockExclusive indeed will not work before Windows 7 (but who cares). Anyway, that part should be rewritten in order to be compilable by legacy visual studios (like vs 2005).

And it is not consistent:
On Windows (with your patch) we have in class RWLock
BOOLEAN trywrlock()
and otherwise
int trywrlock()
The function should not have different return types depending on the system.

Completely agree about consistency. As I've stated, didn't have much time since my changes with bug in exiv2 were in production so I wanted to fix that asap and afterwards sent my patch here.

How can I have access to the repo to fix all those things and not sending you "patches via email"? Are there in SVN world any kind of pull requests?

Taras:

I can't grant you write access to the repos. The credentials to grant write access are retained by Andreas and he likes new contributors to provide patches while we get to know each other.

At the moment, MSVC 2005/8/10/12 builds are broken on the trunk by r4308. We should fix or revert this. I can setup a branch for rwlock development and when the branch builds and passes the test suite on all platforms, I'll merge to trunk.

I agree with T about consistency. BOOLEAN is a Windows 1.0 relic. It's a macro (or typedef) for unsigned short int or something. I would prefer that you return bool, even if it is Internal and not exposed as a public API.

https://bitbucket.org/ribtoks/xpiks-deps/commits/10ae89e8fca5e472a5779ad30398da74e9ccb0f7 - this is the patch with consolidation of api and workaround for Windows < 7

Why do you guys care to support Windows Vista and XP? They are not supported by MS anymore..
(from the other point, why do I even ask.. you still have VS 2003 and SVN..)

Thanks, Taras. I'll look at that this evening. Suggestion:

bool trywrlock()
            {
                return 0 != TryAcquireSRWLockExclusive(&rwlock_);
            }

bool trywrlock()
            {
                return TryAcquireSRWLockExclusive(&rwlock_) == TRUE;
            }

GIT vs SVN

http://dev.exiv2.org/boards/3/topics/1786?r=1788#message-1788

If you join me in Exiv2, I will agree to use GIT for v0.27 IF you promise to help me when I get stuck in GIT hell.

Old Versions of Visual Studio:

If we can build on old versions of Visual Studio, I'm happy to continue to support them. When I was contracting in Silicon Valley, I was rather surprised to discover ancient old tools being used on legacy systems (Dev Studio 6.0 and the like). I don't want to encourage this, however if it's not painful to support 2005/8/10/12, let's keep them alive. v0.26 will be the final release that support Visual Studio .Net (2003).

I don't agree with your proposed change.
TRUE is defined as 1 in wintypes.h

#define FALSE 0
#define TRUE 1

while the MSDN about TryAcquireSRWLockShared says: https://msdn.microsoft.com/en-us/library/windows/desktop/dd405524%28v=vs.85%29.aspx

*Return value*
If the lock is successfully acquired, the return value is nonzero.

Obviosly, nonzero != 1 and I would leave it as is for the compatibility with the official documentation.

Such a discussion about method which is even not used. It will make more sense to remove it until it will be used.

Git stuff. Please define "help". I can offer my help in setting up and maintaining git repo at GitHub + continuous integration for Windows (AppVeyor) and Linux/OS X (Travis CI). However I'm not sure what do you mean by "help".

I also would like you to understand, that I'm not saying that "Git is super the best. Just use Git". What I'm saying instead is that nowadays there're a lot of usefully interconnected services which in most cases work over Git(Hub). Also accepting and managing contributions in there is so much easier .

(Besides that Git seems to have it's own pros and cons comparatively to SVN and seems to be more useful for me, but it's just my point of view)

What do I mean by "Help?". Folks get into trouble with GIT. I've worked with folks who use GIT and they say "It's months of mental fog before you 'get git'". Within a few commands you can be lost and have no idea of the state of your code. So they pull down a fresh copy from the repos. I'm a little afraid that I'll loose control of the code. There is no "central" server. So if I'm in trouble, I don't know from where to pull down a new copy.

SVN is very stable and I have nice tools (SmartSVN). I have purchased a license for SmartGIT. I want to concentrate on Exiv2 development and avoid battles with the source code control system.

My thoughts about moving to GIT are to get involved with a well established project that uses it. I'll come up to speed in the company of more experience GIT users. I'm nervous to drag Exiv2 down this road on my own.

I've misunderstood the return value of that function. You are right and I am wrong. Apologies.

Regarding what do you say about "lost in few commands" - my experience with Git is directly opposite. For me with Git it's always clear where you are.

In my opinion GUI "tools/helpers" for git are the things that will mislead you in the first place since the reason you're using them is that you don't want to bring changes in your life and have exactly UI as you've had with SVN and don't care that Git is underneath. As for me, I didn't ever use any third-party tool for git except of the tools which go with standard git distribution and don't feed need in one.

I've just been recently through move from SVN to Git in quite a big organization with people working with SVN half of their lifes. It was painful for them. But now, after 3 months, majority of them sometimes say "huh, this or that is so much easier now".

Anyway, I will be able to help you with porting exiv2 from svn to git when you will ask particular questions (better in private, of course, since discussion of this issue went too far). To be confident with Git I can suggest you to read only one book "Pro Git" and I hope it will be enough.

Yes. I bought an e-book license for "Pro Git". Thanks for being willing to help with Git. I have no intention to use Git as it will consume lots of my time that is better spent working on exiv2.

Thanks for your Windows Patch for r4308. I'll review that and submit it after dinner.

Thank You, Robin for putting in the effort to get my patch into libexiv2.

I have received the following email from Taras:

Unfortunately, I've found a bug in my patch. 1 odd lock aquisition.
https://bitbucket.org/ribtoks/xpiks-deps/commits/f4ddc6129174515dbe87043a9c9669982f1e2a62 - link to a 1-line patch for a patch.

If that's not gonna work for you, the patch is to remove the line:

Internal::ScopedReadLock srl(rwLock_);

in file properties.cpp in function

const XmpNsInfo* XmpProperties::nsInfoUnsafe(const std::string& prefix)

And my reply:

Thank you for the patch which has been submitted as r4350.