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

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.