25 Mar
2012
25 Mar
'12
11:37 p.m.
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
noticed some problems:
1. PreemtiveLockProvider.lockBlockingThread is concurrently accessed and
updated despite being an unsynchronized HashMap. It must be 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.
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.
4. Related to 3., the 'owners' stack in the lock is concurrently
accessed without synchronization in 'currentThreadOwnsAllLocks'.
As of deadlock detection, do we really need it? If so, would
java.lang.management.ThreadMXBean.findDeadlockedThreads() be sufficient?
In my opinion, the implementation PreemtiveLock should be discarded and
the LockProvider should return an ordinary ReentrantReadWriteLock.
Best Regards,
/Andreas
26 Mar
26 Mar
11:22 p.m.
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
devs(a)xwiki.org
http://lists.xwiki.org/mailman/listinfo/devs
27 Mar
27 Mar
11:32 a.m.
2012-03-26 23:22, Caleb James DeLisle skrev:
The tryLock method is synchronized on the particular instance, but the
lockBlockingThread map and the owner stacks are accessed from other lock
instances. A quick fix to 1., 3., and 4. would be to instead wrap the
tryLock method and the lock method in a
'synchronized (PreemtiveLock.class)' block. (It seems that
that this was the expected behavior, anyway.)
I think this limitation should be more clearly documented. And also the
default lock provider maybe should not return this lock.
Best regards,
/Andreas
On 03/25/2012 05:37 PM, Andreas Jonsson wrote:
I'm using OpenJDK on kvm on a Core 2 Duo.
I tried to debug the test code and I think I found the reason that it is
hanging: in 'reentrenceTest' there is a race condition in the use of the
waitFor/trigger mechanism. The main thread calls 'trigger' before the
spawned thread calls 'waitFor'.
But the tests also looks strange otherwise. The threads spawned by
'lockingTest' may not have completed before 'reentrenceTest' is running
(they are actually deadlocked for the same reason as the threads in
'reentrenceTest', when I run the test), and they are also using the
aliceState and bobState variables, which are not reinitialized before
'reentrenceTest' is running.
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
devs(a)xwiki.org
http://lists.xwiki.org/mailman/listinfo/devs
_______________________________________________
devs mailing list
devs(a)xwiki.org
http://lists.xwiki.org/mailman/listinfo/devs
2:37 p.m.
2012-03-27 11:32, Andreas Jonsson skrev:
2012-03-26 23:22, Caleb James DeLisle skrev:
>
>
> 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.
I think that the fact that the synchronization is made on the instance
and not on the class explains both XWIKI-6595, XWIKI-6878: due to the
race condition occuring, a thread may pop the wrong owner when unlocking
the tread, leaving the victimized thread stuck in an infinite loop, as
it will never become the owner of the lock, the owner stack will never
become empty, and no deadlock will occur.
Best Regards,
/Andreas
I'm using OpenJDK on kvm on a Core 2 Duo.
I tried to debug the test code and I think I found the reason that it is
hanging: in 'reentrenceTest' there is a race condition in the use of the
waitFor/trigger mechanism. The main thread calls 'trigger' before the
spawned thread calls 'waitFor'.
But the tests also looks strange otherwise. The threads spawned by
'lockingTest' may not have completed before 'reentrenceTest' is running
(they are actually deadlocked for the same reason as the threads in
'reentrenceTest', when I run the test), and they are also using the
aliceState and bobState variables, which are not reinitialized before
'reentrenceTest' is running.
The tryLock method is synchronized on the particular instance, but the
lockBlockingThread map and the owner stacks are accessed from other lock
instances. A quick fix to 1., 3., and 4. would be to instead wrap the
tryLock method and the lock method in a
'synchronized (PreemtiveLock.class)' block. (It seems that
that this was the expected behavior, anyway.)
I think this limitation should be more clearly documented. And also the
default lock provider maybe should not return this lock.
Best regards,
/Andreas
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
devs(a)xwiki.org
http://lists.xwiki.org/mailman/listinfo/devs
_______________________________________________
devs mailing list
devs(a)xwiki.org
http://lists.xwiki.org/mailman/listinfo/devs
4556
days inactive
4558
days old
3 comments
2 participants
participants (2)
-
Andreas Jonsson -
Caleb James DeLisle