Re: [hibernate-dev] OGM: CheckStyle and imports due to JavaDoc comments
Does the concept of imports apply at runtime at all, or is this concept not only present at compile time, making life easier by allowing to refer to types using their simple name? In [1] it says about type names in class files: "Class and interface names that appear in class file structures are always represented in a fully qualified form known as binary names". I also can't see any part of the class file structure which would hold information about imports. So when an imported type is not used in the actual code, I don't think it will be referenced in the resulting class file and thus can't cause any issues when not being present. [1] http://docs.oracle.com/javase/specs/jvms/se7/html/jvms-4.html#jvms-4.2.1 2013/8/1 Emmanuel Bernard > > 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
Re: [hibernate-dev] OGM: CheckStyle and imports due to JavaDoc comments
Let's relax this, the likelyhood of this introducing a real problem affecting the runtime is extremely low, and there are better ways to prevent that. I've reopened the issue. Longer reasoning + better proposal: - we're arguing only about things used in javadoc but not code, this: -- makes the probability to trigger very small -- doesn't save us from screw ups anyway - IDEs don't help -> very annoying to find it out at pull request time - it is only a problem if you're loading an optional dependency which shouln't be required for that specific component -- again restricts the application very much -- how likely is it that you're referencing a class which is so much out of context to not be even a dependency of the current class? Now to restrict these chances to zero - as I know some of us are more perfectionists - I actually believe we should simply eradicate any concept optional dependency. The whole concept, as defined by Maven, is unreliable and tricky to handle. We could explore lots of better ways, for example: # different Maven modules keeping the code dependency isolated to a specific module # different source directories in the same M.Module to keep the source unit isolated + check with checkstyle what is legal to be imported with different rules on each path # checkstyle could also be setup to let you import a specific external package only from a specific package (example: make it illegal to import any org.jgroups.* stuff unless you are in package org.hibernate.search.backends.impl.jgroups.* ) .. I personally like the Maven module split better. I'd like to stress that we usually have a 1:1 match between the Maven artifacts that we produce during a release from modular source projects. This doesn't have to be the case, I think we could output jar files which make sense to a consumer which don't necessarily match 1:1 the source structure. What I'm getting to is that code blocks which use Apache Avro should have their own isolated module, as the code which uses JGroups, or those depending to JPA (optional, this one gets tricky..); but we could (on a case by case) still wrap it all up in a single unit for the consumer if we don't want to expose a fragmented project. However we do that, the javadoc safety measure is so small that it's almost pointless. For the record however I'm pretty sure that some older JVMs would have a problem with such a missing class definition, but I didn't say it would actually load the class: that's a different phase of linkage, as it needs to prevent circularity references. Sanne On 1 August 2013 09:36, Gunnar Morling wrote: > Does the concept of imports apply at runtime at all, or is this concept not > only present at compile time, making life easier by allowing to refer to > types using their simple name? In [1] it says about type names in class > files: > > "Class and interface names that appear in class file structures are always > represented in a fully qualified form known as binary names". > > I also can't see any part of the class file structure which would hold > information about imports. So when an imported type is not used in the > actual code, I don't think it will be referenced in the resulting class > file and thus can't cause any issues when not being present. > > [1] http://docs.oracle.com/javase/specs/jvms/se7/html/jvms-4.html#jvms-4.2.1 > > > > 2013/8/1 Emmanuel Bernard > >> >> 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 expe
Re: [hibernate-dev] OGM: CheckStyle and imports due to JavaDoc comments
On 1 Jan 2013, at 8:46 AM, Emmanuel Bernard wrote: > 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. I did. It might have not been Hibernate, but I've seen it happening. ___ 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 1 Jan 2013, at 10:58 AM, Sanne Grinovero wrote: > Let's relax this, the likelyhood of this introducing a real problem > affecting the runtime is extremely low, and there are better ways to > prevent that. I've reopened the issue. Why? The few occasions where it happens a fully qualified class name does not hurt. It is not enough that it is considered bad practise and even Checkstyle for that reason does not allow it in the UnusedImports check? Now we can argue forward and backward about what the compiler does, or how the byte code looks like or what a JVM does or does not do, fact there is a potential risk. Why not stay on the safe side. > - we're arguing only about things used in javadoc but not code, this: > -- makes the probability to trigger very small and it makes the occasions where I have to specify a fully qualified name in the javadocs also rare. In most cases the simple name be enough. > -- doesn't save us from screw ups anyway ? > - IDEs don't help -> very annoying to find it out at pull request time It does. Idea actually warns me about this case. > - it is only a problem if you're loading an optional dependency which > shouln't be required for that specific component > -- again restricts the application very much ? > -- how likely is it that you're referencing a class which is so > much out of context to not be even a dependency of the current class? In Validator we have optional library integration with Jodatime, JSoup and Paranamer. Validator behaves differently or offers additional functionality in case any of the libraries is on the classpath. So I find the use case quite real. > Now to restrict these chances to zero - as I know some of us are more > perfectionists - I actually believe we should simply eradicate any > concept optional dependency. > The whole concept, as defined by Maven, > is unreliable and tricky to handle. +1 I agree, optional dependencies are badly handled by Maven. One of its drawbacks. Does it make the use case of an "optional" dependency less useful? IMO no > We could explore lots of better > ways, for example: > # different Maven modules keeping the code dependency isolated to a > specific module I don't think that scales, see the HV case. > # different source directories in the same M.Module to keep the > source unit isolated + check with checkstyle what is legal to be > imported with different rules on each path -1 That's for sure not the Maven way > I personally like the Maven module split better. I'd like to stress > that we usually have a 1:1 match between the Maven artifacts that we > produce during a release from modular source projects. This doesn't > have to be the case, I think we could output jar files which make > sense to a consumer which don't necessarily match 1:1 the source > structure. -1 > What I'm getting to is that code blocks which use Apache Avro should > have their own isolated module, as the code which uses JGroups, or > those depending to JPA (optional, this one gets tricky..); but we > could (on a case by case) still wrap it all up in a single unit for > the consumer if we don't want to expose a fragmented project. Ok, so you are talking about creating a ueberjar using for example the shade plugin. Mind you that from a productisation point of view we are not supposed to use this plugin (even though the real problem is relocating classes which would not be the case here). Regarding modularisation, I actually support the idea of a separate serialisation module and maybe a module for remote backends (which would depend on the serialisation module). However, I think it should be done as proper modules, meaning if you want to have an ORM based app where you want Search you will need the Search engine, orm, serialisation and remote-backends (the last two modules are made up atm) modules. > For the record however I'm pretty sure that some older JVMs would have > a problem with such a missing class definition, but I didn't say it > would actually load the class: that's a different phase of linkage, as > it needs to prevent circularity references. So why not be safe than sorry? --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
On 1 August 2013 10:41, Hardy Ferentschik wrote: [...] > > So why not be safe than sorry? > > --Hardy That's my same point, I just want to broaden the healthcheck, and am showing you other practical benefits from it, which happen IMHO to also have less annoying checks as a side effect, but that doesn't lessen the safety of my proposal. Enforcing this rule on javadocs prevents a very very unlikely case of being an actual problem as it address a subset of the problematic cases. We have far worse problems when the import is actually being used, and I'm proposing a clean solution for that, which makes this rule redundant. Since it also happens to be inconvenient, I'd rather go for the clean solution ;-) I didn't talk about Shade; look at what I did in Infinispan to build a single module depending on Lucene4 and Lucene3 at the same time (which is impossible in Maven): https://github.com/infinispan/infinispan/blob/master/lucene/lucene-directory/pom.xml Production is fine with that, in fact this is already in products and supported to customers. Shade is more intrusive as it allows you to change the package names; merging a zip file is trivial business. BTW I'm not recommending this strategy as a general solution. Just saying that there are much better - cleaner and safer - solutions to safebelt the optional import if you really care. Cheers, Sanne ___ 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 Thu 2013-08-01 11:41, Hardy Ferentschik wrote: > Why? The few occasions where it happens a fully qualified class name does not > hurt. > It is not enough that it is considered bad practise and even Checkstyle for > that > reason does not allow it in the UnusedImports check? I actually do find it does hurt to have the fqcn in the JavaDoc as reading the JavaDoc from the source is probably 50% of the usage. Does it hurt enough that it outweighs the possible violation? I think it does. Note that the author of this rule did consider it bad practice but either wasn't sure enough or had enough backlash that he did put an option to change the behavior :) ___ 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
>> - 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? We could of thrown an exception if more than one was registered under that name but Emmanuel had a good concern about how that would break Hibernate applications that are clustered in some platforms (as well as applications that serialize/deserialize the EntityManagerFactory). Instead of throwing an exception when multiple EMFs are added under the same name, we log a warning and track the multiple EMFs registered under a particular name. However, we do throw an exception only when the attempt to deserialize the EMF by name but there are more than one EMFs registered (see EntityManagerFactoryRegistry.getNamedEntityManagerFactory(String name)). The distinction is that applications might create multiple EMFs with the same name and we allow that but will throw an error, for the smaller set of apps that actually try to deserialize the EMF by name. It does look like EntityManagerFactoryRegistry.getNamedEntityManagerFactory(String name), is reading an unprotected Set that could be updated concurrently from a different thread. Any other feedback about this thread safety bug before I create a HHH jira? Scott ___ hibernate-dev mailing list hibernate-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev
[hibernate-dev] New stored procedure support
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. ___ hibernate-dev mailing list hibernate-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev
[hibernate-dev] IRC developer meeting - 8/1
Discussed OGM development, progress on the new website design, and other topics.. [11:17] Meeting ended Thu Aug 1 16:16:57 2013 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4) [11:17] Minutes: http://transcripts.jboss.org/meeting/irc.freenode.org/hibernate-dev/2013/hibernate-dev.2013-08-01-15.01.html [11:17] Minutes (text): http://transcripts.jboss.org/meeting/irc.freenode.org/hibernate-dev/2013/hibernate-dev.2013-08-01-15.01.txt [11:17] Log: http://transcripts.jboss.org/meeting/irc.freenode.org/hibernate-dev/2013/hibernate-dev.2013-08-01-15.01.log.html ___ hibernate-dev mailing list hibernate-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev
Re: [hibernate-dev] New stored procedure support
Overall I am pretty confident you are not using the latest as we discussed on IRC. But some comments inline... On Thu 01 Aug 2013 10:30:04 AM CDT, Christian Bauer wrote: - show quoted text - Yes I think this is still a bug. I'll fix this. Its because of the concept of Returns. I should look closer at the type of the Return > 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... > 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. Like I said, I don't think you are using the latest... > 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. Like I said, I don't think you are using the latest... > 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. Like I said, I don't think you are using the latest... > 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. Like I said, I don't think you are using the latest. Here specifically, look for the new getCurrentReturn() method. So you'd have: ProcedureCall call = ...; int updateCount = ( (UpdateCountReturn) call.getResults().getCurrentReturn() ).getUpdateCount(); We talked already about renaming getResults() to getOutputs() or getProcedureOutputs()... > 7. Note sure how to call/handle REF_CURSOR parameters in either API, > doesn't seem to be implemented. Yes I have not implemented this yet. Need to get a database set up that actually supports this (oracle or postgres are the only 2 I know). If you want to help, this would be a good place to help since you seem to have a working pgsql setup already. I can help in terms of how I think it *should* work... ___ 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
On 08/01/2013 10:41 AM, Scott Marlow wrote: >>> - 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? > > We could of thrown an exception if more than one was registered under > that name but Emmanuel had a good concern about how that would break > Hibernate applications that are clustered in some platforms (as well as > applications that serialize/deserialize the EntityManagerFactory). > > Instead of throwing an exception when multiple EMFs are added under the > same name, we log a warning and track the multiple EMFs registered under > a particular name. > > However, we do throw an exception only when the attempt to deserialize > the EMF by name but there are more than one EMFs registered (see > EntityManagerFactoryRegistry.getNamedEntityManagerFactory(String name)). > > The distinction is that applications might create multiple EMFs with the > same name and we allow that but will throw an error, for the smaller set > of apps that actually try to deserialize the EMF by name. > > It does look like > EntityManagerFactoryRegistry.getNamedEntityManagerFactory(String name), > is reading an unprotected Set that could be updated concurrently from a > different thread. > > Any other feedback about this thread safety bug before I create a HHH jira? HHH-8406 is for fixing the thread safety issue. > > Scott > ___ > 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] New stored procedure support
On 01.08.2013, at 19:01, Steve Ebersole 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. ___ hibernate-dev mailing list hibernate-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev
Re: [hibernate-dev] New stored procedure support
Actually thats easy. The following block in getResultList() should just
go away:
if ( outputs().hasMoreReturns() ) {
outputs().getNextReturn();
}
I just missed removing that when moving to the getCurrentReturn() model.
On 08/01/2013 01:05 PM, Christian Bauer wrote:
> On 01.08.2013, at 19:01, Steve Ebersole 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.
>
>
> ___
> 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] Static analysis report on thread safety of Hibernate
On 1 August 2013 06:54, Steve Ebersole wrote: > 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? Not my comments, but having run the tool I think it refers to org.hibernate.test.annotations.entity.HibernateAnnotationMappingTest The tool is simply highlighting that it's bad practice to catch this exception.. I think we can ignore. > >> - 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... I'm fixing this one: HHH-8407. I don't think the main problem is a concurrent stop (as in that case we could still fail to close some connections) but that the thread invoking close() might not have visibility on the latest version of the pool's content: so a leak is possible even if close() is run after all sessions have been closed. >> - 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". Regarding PooledLoOptimizer, noTenantState is protected by synch(this) on PooledLoOptimizer.generate(AccessCallback) but is also reachable via org.hibernate.id.enhanced.PooledLoOptimizer.getLastSourceValue() which is not using the same guard. This could be resolved changing it to a volatile field, however this is used only by: - org.hibernate.id.enhanced.HiLoOptimizer.getHiValue() - org.hibernate.id.enhanced.HiLoOptimizer.getLastSourceValue() - org.hibernate.id.enhanced.HiLoOptimizer.getLastValue() each of these have comments mentioning their purpose is exclusively for testing, so rather than adding volatile on the field (which would affect execution cost of non-test code too), I'll make the method synchronized. I'm not understanding the reference to QueryTranslatorImpl: AFAIK this is not meant to be threadsafe at all? >> - 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? It was referring to the usage of proxiesByKey in org.hibernate.engine.internal.StatefulPersistenceContext.reassociateProxy(LazyInitializer, HibernateProxy) Looks like it detected that it's a ConcurrentMap and is highlighting that the sequence "if not contains then put" is not atomic. I'm changing this into a "putIfAbsent()" as it's generally more efficient than doing both operations, but I don't think it was a bug as this context is not used by multiple threads right? Another thing we could think about is to not use a ConcurrentMap implementation, as it introduces quite some complexity which is apparently not needed. > >> - 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 It's again referring to a suspicious sequence of operations like "if not contains then put" which are not atomic. There are several cases, noone looks critical to me but for the sake of perfection I'm resolving them like this: https://github.com/Sanne/hibernate-orm/commit/085698a2a6823ef7d04ba00df1ab859df37294c4 What they mean re DataSourceBasedMultiTennantConnectionProviderImpl : that even if we could occasionally fail a get/test/put sequence, at least the instance which we would insert in the map is exactly the same, so even
[hibernate-dev] Commit Often, Perfect Later, Publish Once—Git Best Practices
this seems interesting and useful http://sethrobertson.github.io/GitBestPractices/#read - Best Regards, Strong Liu http://about.me/stliu/bio ___ hibernate-dev mailing list hibernate-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev
