See inline...
On 07/31/2013 06:08 AM, Sanne Grinovero wrote:
# Hibernate:
Probably the best finding is the 'Shared non-thread-safe content'
finding in the class 'EntityManagerFactoryRegistry'. In general, the
inconsistent and mixed synchronisation findings are not very good, but
the (get/)check/put and thread-safe collection consistently guarded
findings look good.
- The ConcurrentModificationException finding looks like the catch was
intentional to test for the absence of an old bug.
I don't understand this
one. Could you explain any more?
- The only Inconsistent Collection Sync finding that looks like it is
a
true positive is 'pool' in the class
'DriverManagerConnectionImplProvider'. However, the unsynchronised
access is in a 'stop()' method and it is not clear whether this can run
concurrently with anything else.
DriverManagerConnectionProviderImpl. Stopping
it should not generally
be run concurrent with other accesses, but can't hurt to synchronize it
anyway...
- Most of the Inconsistent Sync findings are likely false positives
due
to the unsynchronised accesses occurring in what the comments say is
test code. Better findings are on the field 'noTennantState', in the
classes 'LegacyHiLoAlgorithmOptimizer' and 'PooledLoOptimizer', and
the
field 'collectedParameterSpecifications' in the class
'QueryTranslatorImpl'.
Not sure what this means. Looking at
PooledLoOptimizer, for example,
yes I see that noTenantState is generated under sync. Where is it
accessed outside a sync? Maybe I am just not understanding their phrase
"Inconsistent Sync".
- The non-atomic check/put in 'StatefulPersistenceContext'
looks like a
true positive.
Not sure what this means. StatefulPersistenceContext is huge and
has
quite a few maps and quite a few access to those Maps. So (1) which
access(es) is this talking about and/or (2) what exactly is the problem
condition they are trying to describe here?
- The non-atomic get/check/puts all look like true positives, except
for
the ones in 'AuditProcessManager', 'ColumnNameCache', and
'DataSourceBasedMultiTennantConnectionProviderImpl', where the mapping
being stored in the concurrent hash maps looks like it is deterministic.
Not
sure what this means
- The shared non-thread-safe content finding looks like it spots a
symptom of a real bug: in the method 'getNamedEntityManagerFactory', a
hashmap is extracted from a concurrent hash map and read from.
Concurrently, there is the possibility that items are removed from the
hash maps. There is no common lock guarding these accesses. This may
cause, at worst, infinite loops, if the hashmap is accessed in an
inconsistent state.
I actually have no idea why it keeps a Set for each name.
Seems to me
the code ultimately throws an exception anyway if more than one was
registered under that name, so why not just store
name->EntityManagerFactory? Scott?