[hibernate-dev] Static analysis report on thread safety of Hibernate

[Steve Ebersole]

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?