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

[Sanne Grinovero]

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.