Guice-persist: Injection of EntityManager outside a UnitOfWork can lead to never closed EntityManagers

[GoogleCodeExporter]

Form the commit rb7a02b02d81c (Jul 7, 2011), in JpaPersistService.java which is 
a Provider<EntityManager>, in the get() method, we can see that if we don't 
already have an EntityManager stored in the thread local variable, we call the 
begin() method. This method will create a new EntityManager, and store it in 
the thread local variable.

So, if we follow the documentation in the wiki (page "Using JPA", section 
"Using the EntityManager inside transactions"), for each thread, for every 
other action needing an EntityManager, the same instance will always be 
returned, and this EntityManager will never be closed.

This can lead to broken threads, as if for any reason the EntityManager can no 
longer be used (connection closed, etc.), there is no way to replace it.

Note that this issue doesn't happen if the injection of EntityManager is done 
inside an opened UnitOfWork or inside a method annotated with @Transactional. 
In these cases, the EntityManager is already created, and will be destroyed at 
the end of the transaction, or on UnitOfWork close.

While taking a look at the get() method of JpaPersistService, we can also see 
that there is a strange precondition test. As said, if the EntityManager 
doesn't exist, it's created. Juste after that, we check if we have an 
EntityManager. If we don't have any, we throw this error message: "Requested 
EntityManager outside work unit.". Sounds like a bug, we should either remove 
the Precondition test, or remove the begin() call if the EntityManager doesn't 
exist.

So, my suggestion would be either to fix the get() method, or change the 
documentation to tell about this potential issue, and suggest the injection of 
Provider<EntityManager> in every cases.

Original issue reported on code.google.com by h...@anthologique.net on 2 Feb 2013 at 10:40

[GoogleCodeExporter]

Original comment by sberlin on 20 Dec 2013 at 2:16

• Added labels: Component-Persist

[GoogleCodeExporter]

Can I go ahead and make a change like this.
public void begin() {

        if(null == entityManager.get()){
            entityManager.set(emFactory.createEntityManager());
        }
    }

in the class
com.google.inject.persist.jpa.JpaPersistService

to resolve this problem. what other problems it might cause? 

Original comment by Krupa.si...@gmail.com on 25 Nov 2014 at 1:17

[GoogleCodeExporter]

this is the exact error I am seeing in my application:
java.lang.IllegalStateException: Work already begun on this thread. Looks like 
you have called UnitOfWork.begin() twice without a balancing call to end() in 
between. 
com.google.inject.internal.util.$Preconditions.checkState(Preconditions.java:142
) 
com.google.inject.persist.jpa.JpaPersistService.begin(JpaPersistService.java:66)
 com.google.inject.persist.PersistFilter.doFilter(PersistFilter.java:87) 
com.google.inject.servlet.FilterDefinition.doFilter(FilterDefinition.java:163) 
com.google.inject.servlet.FilterChainInvocation.doFilter(FilterChainInvocation.j
ava:58) 
com.google.inject.servlet.ManagedFilterPipeline.dispatch(ManagedFilterPipeline.j
ava:118) com.google.inject.servlet.GuiceFilter.doFilter(GuiceFilter.java:113) 
</pre></p><p>note <u>The full stack trace of the root cause is available in the 
Apache Tomcat/7.0.52 logs.</u></p><HR size="1" noshade="noshade"><h3>Apache 
Tomcat/7.0.52</h3></body></html>

Original comment by Krupa.si...@gmail.com on 25 Nov 2014 at 1:19

[GoogleCodeExporter]

The code.google.com guice project has migrated to GitHub.  This issue site is 
no longer being used.  Please use https://github.com/google/guice/issues/739 
instead.

Original comment by sberlin on 25 Nov 2014 at 1:52

• Changed state: MigratedToGitHub