On 03/25/2012 05:37 PM, Andreas Jonsson wrote: > Hello, > > I tried to build all modules under xwiki-platform-core, but the build > process hanged on PreemtiveLockTest, so I had a look at the code and
This shouldn't happen, what kind of Java VM and processor are you using? This is relevant: http://jira.xwiki.org/browse/XWIKI-6595 but it's something I was never able to fully diagnose nor repeat despite running the test thousands of times on a loop. > noticed some problems: > > 1. PreemtiveLockProvider.lockBlockingThread is concurrently accessed and > updated despite being an unsynchronized HashMap. It must be synchronized. It's only ever accessed from synchronized methods. (isDeadlocked() is only ever called from tryLock() which is synchronized) > > 2. The deadlock recovery algorithm is "if there is a deadlock, ignore > the fact that the lock is locked and proceed." To me this seems > unintuitive. When a deadlock is detected, an exception should be thrown > and the thread aborted. This is because as long as a thread is blocked waiting to get other locks, it is safe for the other thread to proceed, this hinges upon the jobs all following the rule of getting all of their locks before beginning the first piece of work. > > 3. Presumably, the thread which is deprived of its lock is supposed to > at least be blocked until the thieving thread unlocks the lock, but > there is a race condition so both threads may run simultaneously. Where is this? Even if we trash the lock I'd still like to fix it. > > 4. Related to 3., the 'owners' stack in the lock is concurrently > accessed without synchronization in 'currentThreadOwnsAllLocks'. It's only ever called from inside of tryLock() which is synchronized. > > As of deadlock detection, do we really need it? If so, would > java.lang.management.ThreadMXBean.findDeadlockedThreads() be sufficient? This method is painfully slow. > > In my opinion, the implementation PreemtiveLock should be discarded and > the LockProvider should return an ordinary ReentrantReadWriteLock. If we are to make a modification to it, I would suggest removing the locks entirely and relying on the user to have a reasonably modern filesystem which resolves concurrent file access with a "last write wins" rule. I would still like to repair any issues with the provider even if unused. Caleb > > Best Regards, > > /Andreas > _______________________________________________ > devs mailing list > [email protected] > http://lists.xwiki.org/mailman/listinfo/devs > _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
