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

[gissuebot]

From head@anthologique.net on February 02, 2013 05:40:00

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: http://code.google.com/p/google-guice/issues/detail?id=739

[gissuebot]

From sberlin on December 20, 2013 06:16:40

(No comment was entered for this change.)

Labels: Component-Persist

[mikehearn]

I ran into this - the silent creation of EntityManagers outside a UnitOfWork scope due to the implicit get turned out to be hiding a bug where I was doing db access outside a UnitOfWork. It showed up as a failure some time later when the thread got reused and I got the "maybe you're not matching with a call to end" message.

[singavarapu]

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?

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.java:58) com.google.inject.servlet.ManagedFilterPipeline.dispatch(ManagedFilterPipeline.java:118) com.google.inject.servlet.GuiceFilter.doFilter(GuiceFilter.java:113)

note The full stack trace of the root cause is available in the Apache Tomcat/7.0.52 logs.

---

Apache Tomcat/7.0.52

[asharayev]

I have exactly the same concern as @mikehearn has.
The problem is that in the get() method of JpaPersistService there is actually the check we want:
see lines 53-56:

    EntityManager em = entityManager.get();
    Preconditions.checkState(null != em, "Requested EntityManager outside work unit. "
        + "Try calling UnitOfWork.begin() first, or use a PersistFilter if you "
        + "are inside a servlet environment.");

But, two lines up there is another code that makes the checkState() call ridiculous:

    if (!isWorking()) {
      begin();
    }

I see that this is definitely a bug. Could it be fixed?

[krdlab]

I also have exactly the same concern. I suppose that the get() should not call the begin() implicitly.

[asharayev]

It seems that you did exactly what I was going to do? Maybe you can create a pull request for this issue, so that somebody from moderators will take a closer look at it?

[stickfigure]

Just hit this and wasted several hours trying to hunt it down. Would love to see that begin() statement nuked!