Static analysis report on thread safety of Hibernate
by 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.
12 years, 1 month
Re: [hibernate-dev] New stored procedure support
by Christian Bauer
On 01.08.2013, at 19:01, Steve Ebersole <steven.ebersole(a)gmail.com> wrote:
>> 2. If there is only one ResultSet returned by the SP, I should be able to call StoredProcedureQuery#getResultList() without first calling hasMoreResults(). This maps to JDBC CallableStatement#excuteQuery().
>
> Like I said, I don't think you are using the latest…
On SNAPSHOT:
org.hibernate.result.NoMoreReturnsException: Results have been exhausted
at org.hibernate.result.internal.ResultImpl.getNextReturn(ResultImpl.java:126)
at org.hibernate.jpa.internal.StoredProcedureQueryImpl.getResultList(StoredProcedureQueryImpl.java:256)
Looks like until issue 1 from my original list is fixed I can't access anything on MySQL properly. I have to call hasMoreResults() before getResultList(), and I'll get an exception when the always present update count return is reached with getResultList(). So I depend on hasMoreResults() not returning true until it really is a ResultSet.
12 years, 1 month
New stored procedure support
by Christian Bauer
Some of the issues I've found with JPA 2.1 and the overall new stored procedure APIs:
1. StoredProcedureQuery#hasMoreResults() returns true if the next result is an update count, it should return false, according to API documentation. On MySQL, if a SP returns a single or multiple ResultSets, you also get an update count of 0. Internally it simply returns row_count() for the last SQL statement in the procedure if you invoke CallableStatement#getUpdateCount().
2. If there is only one ResultSet returned by the SP, I should be able to call StoredProcedureQuery#getResultList() without first calling hasMoreResults(). This maps to JDBC CallableStatement#excuteQuery().
3. When I try to read an OUT argument at index 2 with StoredProcedureQuery#getOutputParameterValue(), it only works if I get it with index 1. This seems like a simple offset bug.
4. Calling an SP which only returns an update count, I should be able to use only StoredProcedureQuery#executeUpdate(). This works in plain JDBC, currently I get an IllegalStateException because I haven't called hasMoreReturns() before.
5. Calling an SP which only returns an update count, the StoredProcedureQuery#execute() method should return false, it currently returns true, even if there is no ResultSet.
6. Calling an SP which only returns an update count is awkward with the Hibernate StoredProcedureCall API, involving several lines and casts to get the update count.
7. Note sure how to call/handle REF_CURSOR parameters in either API, doesn't seem to be implemented.
12 years, 1 month
OGM: CheckStyle and imports due to JavaDoc comments
by Gunnar Morling
Hi,
Currently CheckStyle raises an error due to an "unused import" if a class
imports types which are only referenced in JavaDoc comments. This issue
occurs for instance when referring to a super type in the comments while
only sub-types are used in the actual code:
/**
* This factory creates {@link Service} objects.
*/
public class ServiceFactory {
FooService getFooService() { ... }
}
Another example is "high-level" documentation on a central type of an API
(e.g. its entry point) which refers to types actually used by specific
parts of the API but not the entry point itself. In that case it can still
make sense to mention these types in the high-level docs.
To work around the issue one could use the FQN in the JavaDoc or just
format it as {@code}, but both makes up for less readable documentation IMO.
Personally I don't see a problem with this kind of import and thus suggest
to loosen that CS rule accordingly (it can be configured to take JavaDocs
into account). WDYT?
--Gunnar
12 years, 1 month
