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

[Steve Ebersole]

I can look after I get done mucking around with the JPA TCK...

On 07/31/2013 06:08 AM, Sanne Grinovero wrote:
> Hi all,
> the ThreadSafe team (http://contemplateltd.com) kindly offered a trial
> license to inspect Hibernate and Infinispan projects for code
> correctness; you can think of this as the "Findbugs for concurrent
> code". The idea is to mutually benefit from it: we get to use the
> tool, but we should also provide feedback to the dev team.
>
> Even better than just providing me with a key, they where kind enough
> to run it themselves on the Hibernate ORM and Hibernate Search
> codebases, and what follows is their own opinion; note that while they
> are expert in using the tool & in concurrency problems they need our
> help to verify if these are real bugs and eventually prioritize
> action.
> For example I see a comment about a non-safe usage of a
> PersistenceContext; AFAIK that's not supposed to be thread-safe so
> should not be a problem..
>
> I'll lead the analysis of the Hibernate Search report myself; could
> someone else please take the lead on the ORM report over; still if
> something isn't clear I'm glad to be involved, and please send me some
> feedback that I can then share to the development team of the tool.
>
> Sad note: please don't ask me to share the license, I'm not allowed,
> but I'll be happy to run it on other projects as well and share
> reports; we have until the 30th of August.
>
> A verbatim copy of the comments from the ThreadSafe team follow below.
> Cheers,
> Sanne
>
>
> # 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.
>
> - The putIfAbsent findings all look like false positives, generally
>    because the object construction part looks too heavyweight to benefit
>    from putIfAbsent.
>
> - 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.
>
> - 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'.
>
> - All of the Mixed Synchronisation findings are unfortunately false
>    positives, due to the guard infererence heuristics failing.
>
> - The non-atomic check/put in 'StatefulPersistenceContext' looks like a
>    true positive.
>
> - 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.
>
> - 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.
>
> - All the Thread-safe collection consistently guarded findings look
>    good.
>
> # Hibernate Search
>
> The best findings here are the asynchronous callback ones, while the
> inconsistent sync ones are all likely false positives, due to lifecycle
> constraints.
>
> - The putIfAbsent findings are generally best ignored here.
>
> - The inconsistent synchronisation findings are all likely false
>    positives, due to lifecycle constraints.
>
> - The non atomic check/put findings both look good.
>
> - The Unsynchronised write to field from asynchronous callback findings
>    are all symptoms of the same underlying bug: the execution of the
>    stop() method may overlap with the lazily performed initialisation.
> _______________________________________________
> hibernate-dev mailing list
> hibernate-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/hibernate-dev