Hi, I agree with.
I'll open a new issue and link to this PR and your comment as well. Vlad On Thu, Apr 28, 2016 at 1:05 PM, Sanne Grinovero <sa...@hibernate.org> wrote: > Hi Vlad, > > there are several components which can safely use static fields, of > course as you say only if their state is not affected by which > SessionFactory is using them. > > So I would agree on treating those with care: it should always be a > warning sign during code reviews and warrant an in-depth check, but > ultimately it's a reasonable way to save some resources (both CPU and > memory) - especially when the same JVM runs multiple SessionFactory > instances. > Sure, savings might be small but when the component is clearly > stateless: why not. > > I would assume this is a technical detail which we can handle without > needing to make a blanket statement. > > +1 to avoid such tricks when it's not trivial to assert its safety, > yet there are valid use cases. > > > Looking at the specific case: > the usage of "private static Method memberMethod;" [1] seems dodgy indeed. > Synchronization is clearly not the best solution, as it would create a > contention point were there's no need to create one. It would even > contend across multiple SessionFactory instances which is worse than > usual :) > > Indeed the code is "racy" as different threads might need to repeat > the effort of finding the reference to "memberMethod", it would have > been better to make the field "volatile" to ensure different threads > would see it; > yet the code is not functionally incorrect as any thread invoking that > method will behave the same, and retrieve the right value. > > Better yet than volatile, make the "memberMethod" field Final, have it > initialized statically and move it to the top.. not sure why there are > fields hiding in the mid of a class :) > > BTW this is a good example of sane usage of static (final) fields: it > doesn't matter which SessionFactory is running or how it's configured, > but it's a good thing that different SessionFactory instances won't > have to repeat the initialization of that field over and over. > > Thanks, > Sanne > > 1 - > https://github.com/hibernate/hibernate-orm/pull/1198/files#diff-9f3d2f3cd0828afe20be0db68694f7a0L151 > > On 28 April 2016 at 09:58, Vlad Mihalcea <mihalcea.v...@gmail.com> wrote: > > Hi, > > > > While reviewing this PR: > > > > https://github.com/hibernate/hibernate-orm/pull/1198/files > > > > I realized that we have some static fields which might cause some > problems > > when we deploy two or more SessionFactories in the same container (e.g. > > Spring). > > The fix suggests that we should use synchronised, as indicated by Sonar, > > but I think we should avoid the static fields altogether. > > > > Every instance variable must be confined to an object that's confined to > > the same SessionFactory root object. > > Without static fields, we can make sure that nothing escapes a given > > SessionFactory. > > > > Let me know what you think. > > > > Vlad > > _______________________________________________ > > 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
