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(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev

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

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?