Timing bug in TCK test case ThreadSafe

• Key: JDO-238
• Type: Bug
• Status: Resolved
• Resolution: Fixed
• Priority: Minor
• Assignee: Martin Zaun
• Reporter: Michael Bouschen
• Component/s: tck11, tck2
• Affects Version/s: JDO 2 beta
• Fix Version/s: JDO 2 final

Description

The TCK test ThreadSafe runs multiple threads, where each thread tries to persist the same pc instance using its own PM. The expected behavior is that one thread succeeds persisting the pc instance and stores it at transaction commit. All other threads should result in a JDOException because the pc instance is already bound to a different PM. All threads close the PM at the end.

Now, it might happen that the succeeding thread closes the PM before a parallel thread tries to persist the pc instance. The behavior of pm.makePersistence for a pc instance bound to a closed pm is not specified, so it does not necessarily result in an exception.

The test case should be changed such that the succeeding thread waits for all the other threads before closing the PM. Please note, the solution must be robust enough to avoid a deadlock situation even if an erroneous JDO implementation would allow multiple threads to succeed persisting the pc instance.

Comments

[Martin Zaun]

The described issue is to be fixed by putting a 'barrier' synchronization before the pm.close(). There's a barrier utility that comes with JDK5 (in java.util.concurrent.CyclicBarrier), which cannot be used here to avoid introducing such a dependency. So, I've written a simple (and limited) barrier class that will do for the TCK tests.

That barrier utility class is to be used from both, tck11's and tck20's
  org.apache.jdo.tck.api.persistencemanager.ThreadSafe
tests. So, unless there's a common directory for shared utilities, I'm going to duplicate the barrier source into both, tck11's and tck20's, test classes.

[Martin Zaun]

The barrier class is likely to be used in other TCK tests, I'm therefore putting it into:
tck20/src/java/org/apache/jdo/tck/util and tck11/test/java/org/apache/jdo/tck/util.

Reviewing the ThreadSafe test class with Craig, we'd like it to be extended to test 2 thread-safety aspects by 4 tests:
1) Thread-Safety of PersistenceManagerFactory.getPersistenceManager():
        - pm=pmf.getPM(), barrier, pm.close()
        - pm=pmf.getPM(), pm.makePersistent(private PC instance), barrier, pm.close()
    No exceptions must occur.

2) Thread-Safety of PersistenceManager.makePersistent() on non-managed instances:
        - pm.makePersistent(shared transient PC instance), barrier, pm.close()
        - pm.makePersistent(shared detached PC instance), barrier, pm.close()
    Only one thread must succeed; all others must throw exactly a JDOUserException.

We don't intend to test concurrency within in a transaction (shared PM/Tx) at this time.

[Martin Zaun]

Craig L Russell wrote:
>
> I think it's too late to add the requirement for thread-safety for
> makePersistent of the same detachable instance by multiple threads.
>
> So I'd write a separate low priority JIRA for the thread-safe
> makePersistent detachable and mark it for an unknown release. The rest
> of the thread-safe cases should be good to go.

OK. Created JDO-333 for testing thread-safety of operations on detached instances.
Dropping test pm.makePersistent(shared detached PC instance) from ThreadSafe.java.

[Martin Zaun]

Attached is the fixed ThreadSafe test (using a barrier), extended for 3 subtests (see comments).
By request, the entire source + diffs are attached.

[Craig Russell]

Looks good. A few comments.

1. The thread count of 2 doesn't seem to be much of a test. How about 14 or so?

2. The use of a local pmf should be restricted to where you need more than the single PMF that most tests use. Rather, the pmf defined in JDO_Test can be accessed directly without hiding it by the local pmf.

3. The use of testXXX should be avoided as testXXX methods are special cases for JUnit. Suggest calling the testConcurrent something like runConcurrentThreads.

[Martin Zaun]

> 1. The thread count of 2 doesn't seem to be much of a test. How about 14 or so?

Having run my tests with a threadCount of 10, I was surprised to see that there's a reproducable deadlock happening with 20 threads (actually, with more than 16 threads, on my machine).

There seems to be an issue with jpox: From a certain number on of open PMs, even if they don't have an active transaction anymore, any attempt to begin a transaction on other PMs results in an infinite wait. Filing a jpox bug.

To have the TCK running, I'm checking in the ThreadSafe test with threadCount=10.

> 2. The use of a local pmf should be restricted to where you need more than the single PMF that most tests use. Rather, the pmf defined in JDO_Test can be accessed directly without hiding it by the local pmf.

Removed local pmf.

> 3. The use of testXXX should be avoided as testXXX methods are special cases for JUnit. Suggest calling the testConcurrent something like runConcurrentThreads.

Agreed. Renamed method to: runThreads().

[Martin Zaun]

Committed with revision 384593.

[Erik Bengtson]

The C3P0 has a default of maximum 15 connections in the pool. Try to increment this number

org.jpox.connectionPool.maxPoolSize=25

If this does not help, can you provide a thread dump?