Overuse of synchronization in AbstractBrokerFactory

• Key: OPENJPA-161
• Type: Improvement
• Status: Resolved
• Resolution: Fixed
• Assignee: Unassigned
• Reporter: Patrick Linskey
• Component/s: kernel
• Fix Version/s: 0.9.7

Description

AbstractBrokerFactory maintains a Map<Transaction,List<Broker>> that is guarded by synchronized blocks. These synchronized blocks should be removed if possible.

Comments

[Patrick Linskey]

This patch removes the synchronization on _transactional, replacing it with a ConcurrentHashMap and using the ConcurrentHashMap.putIfAbsent() call to safely handle concurrent access. My analysis is that we do not need to synchronize on the lists inside _transactional, since the key that we look up the collection with is a JTA transaction, and JTA does not allow a transaction to be used concurrently in multiple threads.

Barring any objections, I plan to commit this change in the next day or so.

[Abe White]

I think the FIXME part of the patch should be removed/simplified. The patch itself contains comments like the following:

// we don't need to synchronize on brokers since one JTA transaction
// can never be active on multiple concurrent threads.

Yet the FIXME part explicitly guards against having the same transaction on concurrent threads. If guards like this are necessary, then they need to be consistent throughout the patch. But if we agree that the comment above is correct and the same transaction can't be active on multiple threads, then the FIXME part is useless.

[Patrick Linskey]

Oops, FIXME is stale -- the comment is no longer applicable. Disregard. (In an earlier revision, I was guarding 'brokers' with a synchronized block, but analyzed further and decided that indeed, it was not necessary.)

[Abe White]

The code in question (just after the FIXME) is still guarding against the same transaction in concurrent threads. See the "someone beat us to it" putIfAbsent call. If we agree that transactions are thread bound -- as must be the case for the rest of the patch to be correct -- no one can "beat us to it", because we're the only thread with access to that transaction.

[Patrick Linskey]

Good point -- I guess we can switch to just using a put() call instead. I'll change accordingly.

[Patrick Linskey]

New patch that doesn't use putIfAbsent()