Re: [hibernate-dev] Static analysis report on thread safety of Hibernate
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? ___ hibernate-dev mailing list hibernate-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev
Re: [hibernate-dev] OGM: CheckStyle and imports due to JavaDoc comments
For the record, I side with Gunnar in relaxing this. I actually did not see
this thread and opened a issue for it this very morning.
In most situations we reference classes local to the module via {@link} and the
fully qualified class name is very annoying and very long.
On 31 juil. 2013, at 09:33, Gunnar Morling wrote:
> 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
> ___
> hibernate-dev mailing list
> hibernate-dev@lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/hibernate-dev
___
hibernate-dev mailing list
hibernate-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/hibernate-dev
Re: [hibernate-dev] OGM: CheckStyle and imports due to JavaDoc comments
On 31 juil. 2013, at 13:14, Hardy Ferentschik wrote: > > On 31 Jan 2013, at 10:41 AM, Gunnar Morling wrote: > >> Personally I prefer to include a class via fully qualified name if it is >> only used in the javadocs. >> I think the readability does not suffer too much and adding an actual import >> has actually >> runtime consequences. We already had cases where a javadoc import caused a >> hard link >> between code which is otherwise decoupled. >> >> WDYM exactly by "hard link" in this context? Is it about referencing a type >> from an optional dependency which might not be present at runtime? > > Right, optional dependencies. Take JPA and Search. In the engine module we > avoid importing it directly and do processing of > @Id reflectively. Having an import of javax.persitence.Id would create a > runtime dependency. > >> I just tried out this scenario (adding an import statement just for JavaDoc >> to a type which is not present at runtime) and still could execute the >> importing class >> without problems. Only when accessing the imported type in the actual code >> I'm getting a CNFE. But this might be specific to the VM in use, not sure. > > We had this discussion now several times. I remember when we wrote this > reflection code the first time (many years back ;-)) > all referenced classes were eagerly loaded. In this case just having the > import statement required the class being > available at runtime. Obviously this must have changed a bit which also is > proven by your experiment. > Of course my memory might be flawed but the problems I recall around isolating a piece of code for hv, search and later bean validation from core did not involve unused imports. Come to think of it, I have never seen an *unused* import eagerly loading a class even though in practice it's left unspecified by the JLS. ___ hibernate-dev mailing list hibernate-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev
[hibernate-dev] OGM: CheckStyle and imports due to JavaDoc comments
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
___
hibernate-dev mailing list
hibernate-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/hibernate-dev
Re: [hibernate-dev] OGM: CheckStyle and imports due to JavaDoc comments
Hi,
Personally I prefer to include a class via fully qualified name if it is only
used in the javadocs.
I think the readability does not suffer too much and adding an actual import
has actually
runtime consequences. We already had cases where a javadoc import caused a hard
link
between code which is otherwise decoupled.
Even the check stye documentation recommends against it in the configuration
of Unused Imports -
http://checkstyle.sourceforge.net/config_imports.html#UnusedImports.
--Hardy
On 31 Jan 2013, at 9:33 AM, Gunnar Morling wrote:
> 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
> ___
> hibernate-dev mailing list
> hibernate-dev@lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/hibernate-dev
___
hibernate-dev mailing list
hibernate-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/hibernate-dev
Re: [hibernate-dev] OGM: CheckStyle and imports due to JavaDoc comments
I had a very minor preference for declaring the import over writing
verbose javadocs, but Hardy's point on decoupling is very interesting.
+1 to keep the error
On 31 July 2013 09:08, Hardy Ferentschik wrote:
> Hi,
>
> Personally I prefer to include a class via fully qualified name if it is only
> used in the javadocs.
> I think the readability does not suffer too much and adding an actual import
> has actually
> runtime consequences. We already had cases where a javadoc import caused a
> hard link
> between code which is otherwise decoupled.
>
> Even the check stye documentation recommends against it in the configuration
> of Unused Imports -
> http://checkstyle.sourceforge.net/config_imports.html#UnusedImports.
>
> --Hardy
>
>
> On 31 Jan 2013, at 9:33 AM, Gunnar Morling wrote:
>
>> 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
>> ___
>> hibernate-dev mailing list
>> hibernate-dev@lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/hibernate-dev
>
>
> ___
> hibernate-dev mailing list
> hibernate-dev@lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/hibernate-dev
___
hibernate-dev mailing list
hibernate-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/hibernate-dev
Re: [hibernate-dev] OGM: CheckStyle and imports due to JavaDoc comments
2013/7/31 Hardy Ferentschik
> Hi,
>
> Personally I prefer to include a class via fully qualified name if it is
> only used in the javadocs.
> I think the readability does not suffer too much and adding an actual
> import has actually
> runtime consequences. We already had cases where a javadoc import caused a
> hard link
> between code which is otherwise decoupled.
>
WDYM exactly by "hard link" in this context? Is it about referencing a type
from an optional dependency which might not be present at runtime?
I just tried out this scenario (adding an import statement just for JavaDoc
to a type which is not present at runtime) and still could execute the
importing class without problems. Only when accessing the imported type in
the actual code I'm getting a CNFE. But this might be specific to the VM in
use, not sure.
So if you guys are concerned about this case then let's better keep it as
is.
>
> Even the check stye documentation recommends against it in the
> configuration
> of Unused Imports -
> http://checkstyle.sourceforge.net/config_imports.html#UnusedImports.
>
> --Hardy
>
>
> On 31 Jan 2013, at 9:33 AM, Gunnar Morling wrote:
>
> > 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
> > ___
> > hibernate-dev mailing list
> > hibernate-dev@lists.jboss.org
> > https://lists.jboss.org/mailman/listinfo/hibernate-dev
>
>
___
hibernate-dev mailing list
hibernate-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/hibernate-dev
Re: [hibernate-dev] OGM: CheckStyle and imports due to JavaDoc comments
On 31 July 2013 09:41, Gunnar Morling wrote:
> 2013/7/31 Hardy Ferentschik
>
>> Hi,
>>
>> Personally I prefer to include a class via fully qualified name if it is
>> only used in the javadocs.
>> I think the readability does not suffer too much and adding an actual
>> import has actually
>> runtime consequences. We already had cases where a javadoc import caused a
>> hard link
>> between code which is otherwise decoupled.
>>
>
> WDYM exactly by "hard link" in this context? Is it about referencing a type
> from an optional dependency which might not be present at runtime?
>
> I just tried out this scenario (adding an import statement just for JavaDoc
> to a type which is not present at runtime) and still could execute the
> importing class without problems. Only when accessing the imported type in
> the actual code I'm getting a CNFE. But this might be specific to the VM in
> use, not sure.
It is VM specific indeed, and sensitive to situation as to prevent
circularity reference loading it never loads all imports aggressively,
but some might depending on the moon phase :-)
AFAIR IBM's JVM eagerly verifies class definition availability (but
does not necessarily initialize the class), and it wouldn't surprise
me even hotspot's behaviour could depend on specific situations and/or
obscure flags.. I don't really know how to write a comprehensive test
to showcase this but let's admit a simple test isn't enough to cover
all situations.
I'd expect more recent compilers to remove such dead imports, but
given it's not a big deal to remove these in the sources I'd rather be
safe: we had problems with such imports in the past.
I guess ideally we'd want IDEs to warn about it, I haven't seen an
option for this (other than checkstyle integration plugins).
Sanne
>
> So if you guys are concerned about this case then let's better keep it as
> is.
>
>
>
>>
>> Even the check stye documentation recommends against it in the
>> configuration
>> of Unused Imports -
>> http://checkstyle.sourceforge.net/config_imports.html#UnusedImports.
>>
>> --Hardy
>>
>>
>> On 31 Jan 2013, at 9:33 AM, Gunnar Morling wrote:
>>
>> > 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
>> > ___
>> > hibernate-dev mailing list
>> > hibernate-dev@lists.jboss.org
>> > https://lists.jboss.org/mailman/listinfo/hibernate-dev
>>
>>
> ___
> hibernate-dev mailing list
> hibernate-dev@lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/hibernate-dev
___
hibernate-dev mailing list
hibernate-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/hibernate-dev
[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. ___ hibernate-dev mailing list hibernate-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev
Re: [hibernate-dev] OGM: CheckStyle and imports due to JavaDoc comments
On 31 Jan 2013, at 10:41 AM, Gunnar Morling wrote: > Personally I prefer to include a class via fully qualified name if it is only > used in the javadocs. > I think the readability does not suffer too much and adding an actual import > has actually > runtime consequences. We already had cases where a javadoc import caused a > hard link > between code which is otherwise decoupled. > > WDYM exactly by "hard link" in this context? Is it about referencing a type > from an optional dependency which might not be present at runtime? Right, optional dependencies. Take JPA and Search. In the engine module we avoid importing it directly and do processing of @Id reflectively. Having an import of javax.persitence.Id would create a runtime dependency. > I just tried out this scenario (adding an import statement just for JavaDoc > to a type which is not present at runtime) and still could execute the > importing class > without problems. Only when accessing the imported type in the actual code > I'm getting a CNFE. But this might be specific to the VM in use, not sure. We had this discussion now several times. I remember when we wrote this reflection code the first time (many years back ;-)) all referenced classes were eagerly loaded. In this case just having the import statement required the class being available at runtime. Obviously this must have changed a bit which also is proven by your experiment. I think Steve had a similar experience when writing some code related to ORM and JPA I think. However, I cannot find any official reference stating that it is ok to have unused import statement and that they don't have any runtime implications. It seems to be some sort of VM optimisation which would imply that it is VM specific and another implementation might as well load eagerly. --Hardy ___ hibernate-dev mailing list hibernate-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev
[hibernate-dev] Mistake in the JUnit3 cleanup
Hi Hardy, sorry I screwed up both during review and setting up ci.hibernate.org : the code in Maven module _hibernate-search-performance_ isn't compiling anymore; could you extend your refactoring to this module please? This slipped in as the profile is disabled by default; I'm fixing the pull requests checker to verify this profile as well. I suspect I wanted to keep the PR checker quick and lean, but it doesn't take too long now. Cheers, Sanne ___ hibernate-dev mailing list hibernate-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev
Re: [hibernate-dev] Mistake in the JUnit3 cleanup
On 31 Jan 2013, at 1:38 PM, Sanne Grinovero wrote: > Hi Hardy, > sorry I screwed up both during review and setting up ci.hibernate.org > : the code in Maven module _hibernate-search-performance_ isn't > compiling anymore; could you extend your refactoring to this module > please? :-( I can have a look tomorrow > This slipped in as the profile is disabled by default; I'm fixing the > pull requests checker to verify this profile as well. I suspect I > wanted to keep the PR checker quick and lean, but it doesn't take too > long now. And this is exactly why I always say that all modules should build by default (with options for the performance minded to disable certain modules/parts)! Also getting feedback from the pull request checker is ways too late. By the time I get feedback from there I have moved on and work on another branch. At least the code parts needs to be all compiled on each build. But I guess I stand alone with this opinion :-( --Hardy ___ hibernate-dev mailing list hibernate-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev
Re: [hibernate-dev] OGM: CheckStyle and imports due to JavaDoc comments
2013/7/31 Sanne Grinovero
> On 31 July 2013 09:41, Gunnar Morling wrote:
> > 2013/7/31 Hardy Ferentschik
> >
> >> Hi,
> >>
> >> Personally I prefer to include a class via fully qualified name if it is
> >> only used in the javadocs.
> >> I think the readability does not suffer too much and adding an actual
> >> import has actually
> >> runtime consequences. We already had cases where a javadoc import
> caused a
> >> hard link
> >> between code which is otherwise decoupled.
> >>
> >
> > WDYM exactly by "hard link" in this context? Is it about referencing a
> type
> > from an optional dependency which might not be present at runtime?
> >
> > I just tried out this scenario (adding an import statement just for
> JavaDoc
> > to a type which is not present at runtime) and still could execute the
> > importing class without problems. Only when accessing the imported type
> in
> > the actual code I'm getting a CNFE. But this might be specific to the VM
> in
> > use, not sure.
>
> It is VM specific indeed, and sensitive to situation as to prevent
> circularity reference loading it never loads all imports aggressively,
> but some might depending on the moon phase :-)
> AFAIR IBM's JVM eagerly verifies class definition availability (but
> does not necessarily initialize the class), and it wouldn't surprise
> me even hotspot's behaviour could depend on specific situations and/or
> obscure flags.. I don't really know how to write a comprehensive test
> to showcase this but let's admit a simple test isn't enough to cover
> all situations.
>
Makes sense. There wouldn't be a problem in most cases (e.g. when
referencing a class from the same module), but it's better to keep the rule
as is to be on the safe side.
> I'd expect more recent compilers to remove such dead imports, but
> given it's not a big deal to remove these in the sources I'd rather be
> safe: we had problems with such imports in the past.
> I guess ideally we'd want IDEs to warn about it, I haven't seen an
> option for this (other than checkstyle integration plugins).
>
Indeed it's Eclipse which created these imports. There is a feature request
for making this configurable (
https://bugs.eclipse.org/bugs/show_bug.cgi?id=128661) but this is
unresolved.
> Sanne
>
>
> >
> > So if you guys are concerned about this case then let's better keep it as
> > is.
> >
> >
> >
> >>
> >> Even the check stye documentation recommends against it in the
> >> configuration
> >> of Unused Imports -
> >> http://checkstyle.sourceforge.net/config_imports.html#UnusedImports.
> >>
> >> --Hardy
> >>
> >>
> >> On 31 Jan 2013, at 9:33 AM, Gunnar Morling
> wrote:
> >>
> >> > 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
> >> > ___
> >> > hibernate-dev mailing list
> >> > hibernate-dev@lists.jboss.org
> >> > https://lists.jboss.org/mailman/listinfo/hibernate-dev
> >>
> >>
> > ___
> > hibernate-dev mailing list
> > hibernate-dev@lists.jboss.org
> > https://lists.jboss.org/mailman/listinfo/hibernate-dev
> ___
> hibernate-dev mailing list
> hibernate-dev@lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/hibernate-dev
>
___
hibernate-dev mailing list
hibernate-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/hibernate-dev
Re: [hibernate-dev] Mistake in the JUnit3 cleanup
2013/7/31 Hardy Ferentschik > > On 31 Jan 2013, at 1:38 PM, Sanne Grinovero wrote: > > > Hi Hardy, > > sorry I screwed up both during review and setting up ci.hibernate.org > > : the code in Maven module _hibernate-search-performance_ isn't > > compiling anymore; could you extend your refactoring to this module > > please? > > :-( I can have a look tomorrow > > > This slipped in as the profile is disabled by default; I'm fixing the > > pull requests checker to verify this profile as well. I suspect I > > wanted to keep the PR checker quick and lean, but it doesn't take too > > long now. > > And this is exactly why I always say that all modules should build by > default > (with options for the performance minded to disable certain modules/parts)! > > Also getting feedback from the pull request checker is ways too late. By > the time > I get feedback from there I have moved on and work on another branch. > At least the code parts needs to be all compiled on each build. > > But I guess I stand alone with this opinion :-( > No, you don't. Take OGM for example, there we just enabled the MongoDB modules to run as part of the default build. > > --Hardy > > > ___ > hibernate-dev mailing list > hibernate-dev@lists.jboss.org > https://lists.jboss.org/mailman/listinfo/hibernate-dev > ___ hibernate-dev mailing list hibernate-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev
Re: [hibernate-dev] Mistake in the JUnit3 cleanup
On 31 July 2013 13:04, Gunnar Morling wrote: > 2013/7/31 Hardy Ferentschik >> >> >> On 31 Jan 2013, at 1:38 PM, Sanne Grinovero wrote: >> >> > Hi Hardy, >> > sorry I screwed up both during review and setting up ci.hibernate.org >> > : the code in Maven module _hibernate-search-performance_ isn't >> > compiling anymore; could you extend your refactoring to this module >> > please? >> >> :-( I can have a look tomorrow >> >> > This slipped in as the profile is disabled by default; I'm fixing the >> > pull requests checker to verify this profile as well. I suspect I >> > wanted to keep the PR checker quick and lean, but it doesn't take too >> > long now. >> >> And this is exactly why I always say that all modules should build by >> default >> (with options for the performance minded to disable certain >> modules/parts)! >> >> Also getting feedback from the pull request checker is ways too late. By >> the time >> I get feedback from there I have moved on and work on another branch. >> At least the code parts needs to be all compiled on each build. >> >> But I guess I stand alone with this opinion :-( > > > No, you don't. Take OGM for example, there we just enabled the MongoDB > modules to run as part of the default build. It's a tradeoff, it's not the end of the world if this module doesn't compile for day. On the other hand I run the build dozens of times a day, quite happy it takes just 2 minutes, which is not possible with the performance tests. Another approach would be to move those tests into a different repository; but technically I wish I had merged my other tests sooner as now they are outdated and need maintenance. I don't think we should automate this all to make our life too easy: engineers!=mindless chickens. it was my mistake, should just think better of the consequences of code change and I think we can generally do that, and be forgiving for the occasional mistake as it saves lots of time. The MongoDB example is different; I have always run those tests, I just had to put a 10 minutes investment to install MongoDB and setup the variables. The improvement is welcome but I think mostly meant for eventual occasional contributors not interested in the MongoDB backend. Sanne > >> >> >> --Hardy >> >> >> ___ >> hibernate-dev mailing list >> hibernate-dev@lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/hibernate-dev > > ___ hibernate-dev mailing list hibernate-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev
Re: [hibernate-dev] Mistake in the JUnit3 cleanup
> It's a tradeoff, it's not the end of the world if this module doesn't > compile for day. > On the other hand I run the build dozens of times a day, quite happy > it takes just 2 minutes, which is not possible with the performance > tests. I think we have to differentiate here. I am not saying that the performance tests need to run all the time, however, I would like the test classes to at least getting compiled to detect simple compilation errors. Right now the performance module is completely deactivated until a certain profile is selected. IMO the performance module should be part of the default module list and what should be turned on and off via the profile is the actual test run. This way at least the sources get compiled and the costs for that should be quite small. > Another approach would be to move those tests into a different > repository; I don't think this is necessary --Hardy ___ hibernate-dev mailing list hibernate-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev
Re: [hibernate-dev] Static analysis report on thread safety of Hibernate
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@lists.jboss.org > https://lists.jboss.org/mailman/listinfo/hibernate-dev ___ hibernate-dev mailing list hibernate-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev
