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

[Sanne Grinovero]

On 1 August 2013 06:54, Steve Ebersole <steve at hibernate.org> wrote:
> 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?

Not my comments, but having run the tool I think it refers to
org.hibernate.test.annotations.entity.HibernateAnnotationMappingTest

The tool is simply highlighting that it's bad practice to catch this
exception.. I think we can ignore.

>
>> - 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...

I'm fixing this one: HHH-8407.
I don't think the main problem is a concurrent stop (as in that case
we could still fail to close some connections) but that the thread
invoking close() might not have visibility on the latest version of
the pool's content: so a leak is possible even if close() is run after
all sessions have been closed.


>> - 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".

Regarding PooledLoOptimizer, noTenantState is protected by synch(this) on
PooledLoOptimizer.generate(AccessCallback) but is also reachable via
org.hibernate.id.enhanced.PooledLoOptimizer.getLastSourceValue() which
is not using the same guard.
This could be resolved changing it to a volatile field, however this
is used only by:
 - org.hibernate.id.enhanced.HiLoOptimizer.getHiValue()
 - org.hibernate.id.enhanced.HiLoOptimizer.getLastSourceValue()
 - org.hibernate.id.enhanced.HiLoOptimizer.getLastValue()
each of these have comments mentioning their purpose is exclusively
for testing, so rather than adding volatile on the field (which would
affect execution cost of non-test code too), I'll make the method
synchronized.

I'm not understanding the reference to QueryTranslatorImpl: AFAIK this
is not meant to be threadsafe at all?

>> - 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?

It was referring to the usage of proxiesByKey in
org.hibernate.engine.internal.StatefulPersistenceContext.reassociateProxy(LazyInitializer,
HibernateProxy)

Looks like it detected that it's a ConcurrentMap and is highlighting
that the sequence "if not contains then put" is not atomic.
I'm changing this into a "putIfAbsent()" as it's generally more
efficient than doing both operations, but I don't think it was a bug
as this context is not used by multiple threads right?

Another thing we could think about is to not use a ConcurrentMap
implementation, as it introduces quite some complexity which is
apparently not needed.

>
>> - 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

It's again referring to a suspicious sequence of operations like "if
not contains then put" which are not atomic. There are several cases,
noone looks critical to me but for the sake of perfection I'm
resolving them like this:

https://github.com/Sanne/hibernate-orm/commit/085698a2a6823ef7d04ba00df1ab859df37294c4

What they mean re DataSourceBasedMultiTennantConnectionProviderImpl :
that even if we could occasionally fail a get/test/put sequence, at
least the instance which we would insert in the map is exactly the
same, so even if it triggers it doesn't change any state.

>
>> - 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?

I'll add comments to the JIRA opened by Scott, all other things
commented in this thread are either fixed in [1] or rejected as false
positives. There are some more minor things being reported that I'll
look into next week.

https://github.com/hibernate/hibernate-orm/pull/562

-Sanne