Persist Extension: UnitOfWork.begin() throws IllegalStateException when called multiple times

[gissuebot]

From michal.galet on January 31, 2011 06:41:53

Version: guice-persist-3.0-rc2

The implementation of UnitOfWork.begin() throws exception using precondition:

Preconditions.checkState(null == entityManager.get(),
        "Work already begun on this thread. Looks like you have called UnitOfWork.begin() twice"
         + " without a balancing call to end() in between.");

The javadoc and documentation claims that unit of works can be nested and the begin() method can be called multiple times with no harm. Moreover every call to Provider<EntityManager>.get() calls that begin() method if the work of unit is not in progress.

Why the precondition was added?

Original issue: http://code.google.com/p/google-guice/issues/detail?id=597

[gissuebot]

From [email protected] on February 07, 2011 20:12:30

We should probably correct the javadoc.

[gissuebot]

From michal.galet on February 07, 2011 23:46:09

Yes, that would be fine. The limitation that we can not nest unit of works will have impact on users migrating from warp-persist. Would it be possible to add "boolean isWorking()" method to the UnitOfWork interface?

Could you please describe what was the reasoning behind this?

[gissuebot]

From sberlin on February 18, 2011 06:49:05

Issue 604 has been merged into this issue.

[gissuebot]

From sberlin on February 21, 2011 17:45:45

(No comment was entered for this change.)

Labels: Extension-Persist

[gissuebot]

From itscore.de on May 23, 2011 03:43:25

I have the same issue by using Guice and GWT requestfactory which assume to use static entity manager on entity:

@MappedSuperclass
public class BaseEntity implements Serializable {
    private static final long serialVersionUID = 1L;
   
    @Inject
    protected static Provider<EntityManager> emProvider;

    ....

    public static final EntityManager entityManager() {
    return emProvider.get();
    }

    /*
     * (non-Javadoc)
     *
     * @see com.google.inject.AbstractModule#configure()
     */
    @Override
    protected void configure() {
    install(new JpaPersistModule("xxxx"));
    requestStaticInjection(BaseEntity.class);
    }

Which is is the best practice using Guice-Persist extension and GWT requestfactory?

Thank You in advance.

[gissuebot]

From g.zoritchak on May 26, 2011 05:18:47

I had to move back quickly to warp-persist because of this precondition (even if I do not use directly UnitOfWork but only the PersistFilter).

Nobody to answer the questions of the previous comments?

[gissuebot]

From remi.bantos on May 27, 2013 13:16:19

I had the same issue in an application on Tomcat.
I've read this article which helped me to figure out what the problem was: http://barinskis.me/blog/2013/04/30/persistence-unit-of-work-pattern-in-sitebricks/ In my personal case, i had a request thread in Tomcat which was using a connexion for a very long time (a timeout setting was not configured properly in a client stub ...).
During this period, Tomcat DBCP had closed and removed the connexion from pool considering it as abandoned (this is a dbcp common setting).
So the associated unit of work was not properly ended. Therefore, a next request which was reusing the same thread from tomcat pool were doing unitofwork.begin() although an entitymanager were already attached to thread (see JpaPersistService class, its entityManager instance is a thread-local variable)

Hope this help.

I think we could handle that by adding a test in PersistFilter or JpaPersistService to manage this situation.

[gissuebot]

From martins.barinskis on May 28, 2013 00:46:56

I really think that introducing in Guice Persist any kind of checking whether unit of work has already begun is a bad idea:

1. That's the whole point of Unit Of Work pattern: the user is responsible for explicitly starting it and ending it as well. If it fails to do so, that's not a bug in Guice Persist, that's a bug in the user's code or other libraries;
2. If we reuse an existing unit of work, we kind of hijack an existing (most probably not flushed) ORM session which could be a cause for much more obscure bugs than those people are describing here.

[gissuebot]

From [email protected] on May 28, 2013 01:02:53

If you do not want to make the state of the unit of work public then you must prevent the unit of work from being started behind the scene.

This happens in the method
JpaPersistService.get() Which will return the EntityManager. The side effect of this method is that a unit of work is started.

First of all it is bad style to have a getter with a side effect. Rather have the get() throw an IllegalStateException if the unit of work has not been started.

Second most of the programmers are not aware that they are invoking the above method when they have an EntityManager injected into their class. This makes tracking the bug of an already active unit of work a nightmare.

[gissuebot]

From martins.barinskis on May 28, 2013 01:18:38

I completely agree with comment #9, it would be much easier to debug these problems if there would be no implicit unit of work initialization.

[gissuebot]

From remi.bantos on June 06, 2013 06:28:15

I agree also that JPAPersistService should not perform an explicit begin of the unit of work.
However, regarding my personal case, i think there is a real issue in guice-persist that should be fixed and which is linked to this issue.

Indeed, the end() method of JpaPersistService might not work when the jdbc connection is already closed by an external application.
So the test i was talking about in my previous post would be to check that the connection is not already closed before calling the close() method on the EntityManager in JpaPersistService end() method.

(Perhaps i should report another issue linked to this one?)

[gissuebot]

From remi.bantos on June 13, 2013 10:54:44

Hi,

I have cloned git repository and added the following thing:

EntityManager#close() method call in JpaPersistService#end() method is now surrounded by a try{}.
entityManager.remove() is now always performed in a finally.

With this modification, unitOfWork is now properly ended, even if there is RuntimeException exception during close()

[gissuebot]

From twr.npg on July 16, 2013 01:21:19

I encountered the same issue with JpaPersistService.end() and would like it to be addressed. How likely is it that the fix suggested in the previous post would end up in a new release?

Problem:
If an exception is thrown in com.google.inject.persist.jpa.JpaPersistService.end() on em.close(), entityManager.remove() is not called.

When used from com.google.inject.persist.PersistFilter, this results in java.lang.IllegalStateException for every consequent invocation of the filter.

1. exception on em.close()
   org.hibernate.exception.GenericJDBCException: Could not close connection
           at org.hibernate.exception.internal.StandardSQLExceptionConverter.convert(StandardSQLExceptionConverter.java:54)
           at org.hibernate.engine.jdbc.spi.SqlExceptionHelper.convert(SqlExceptionHelper.java:125)
           at org.hibernate.engine.jdbc.spi.SqlExceptionHelper.convert(SqlExceptionHelper.java:110)
           at org.hibernate.engine.jdbc.internal.LogicalConnectionImpl.releaseConnection(LogicalConnectionImpl.java:327)
           at org.hibernate.engine.jdbc.internal.LogicalConnectionImpl.close(LogicalConnectionImpl.java:199)
           at org.hibernate.engine.jdbc.internal.JdbcCoordinatorImpl.close(JdbcCoordinatorImpl.java:141)
           at org.hibernate.engine.transaction.internal.TransactionCoordinatorImpl.close(TransactionCoordinatorImpl.java:276)
           at org.hibernate.internal.SessionImpl.close(SessionImpl.java:352)
           at org.hibernate.ejb.EntityManagerImpl.close(EntityManagerImpl.java:137)
           at com.google.inject.persist.jpa.JpaPersistService.end(JpaPersistService.java:81)
           at com.google.inject.persist.PersistFilter.doFilter(PersistFilter.java:91)

2. exception on filter invocation
   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.
           at com.google.inject.internal.util.$Preconditions.checkState(Preconditions.java:142)
           at com.google.inject.persist.jpa.JpaPersistService.begin(JpaPersistService.java:66)
           at com.google.inject.persist.PersistFilter.doFilter(PersistFilter.java:87)

[gissuebot]

From taeric on October 16, 2013 08:37:47

I am seeing the same behavior as indicated by the above link regarding the PersistFilter.  In my case, I am using Shiro which has some filters that will cause all filters to process a request a second time.  This will include the Persist filter, which will then blow up when it starts the unit of work again.

To "fix" this locally, I implemented a wrapper around the PersistFilter that basically implements a pattern in the Shiro class OncePerRequestFilter.  That is, I check an attribute on the request called "PERSIST.FILTERED" and only call to the wrapped filter if this has not been set to true.

So, a couple of questions:

  - Is this a bad idea or a sign that I have done something else awry?
  - Any reason not to have this logic in the main PersistFilter directly?  (I can put together a pull request easily enough.)

[gissuebot]

From remi.bantos on October 16, 2013 09:26:46

Hi,

I have implemented a fix for this issue as already mentioned above. https://code.google.com/r/remibantos-guice-persist/source/detail?r=3a9d1012fedd403c2aaddc47701f8ace9eaa3eb8 There is also a thread about this behavior here: https://groups.google.com/forum/#!topic/google-guice/n6D0T_yCVyc Some people would like this fix to be reviewed and pulled to master.

Until it is pulled/released, you can eventually add your own JpaPersistService class with this fix to your application in order that it takes precedence other JpaPersistService class from guice-persist library during classloader execution.

[gissuebot]

From taeric on October 16, 2013 09:48:26

So, that does not seem to be quite what I am seeing.  My specific case is as follows:

  - Request comes in
  - FilterChain starts
    - PersistFilter starts a unit of work
      - continues FilterChain
      - ShiroFilter sees I need to login, redirects to login url
        - Filter Chains runs again
          - Persist filter throws exception starting a new unit of work.
    - Finally of the first call to PersistFilter runs, calling UnitOfWork.end()

In my case, the problem is essentially that the PersistFilter is not reentrant.  Simple fix is to make so that it is.  At least, I believe that is my problem.  Again, if I am doing something else wrong, I'm ears.  (And, if I need to open another ticket for this, let me know.)

[gissuebot]

From sberlin on December 20, 2013 06:20:14

(No comment was entered for this change.)

Labels: -Extension-Persist Component-Persist

[gissuebot]

From fjackstadt on June 20, 2014 05:05:21

We need the fix of remi.bantos very urgently. Please merge it into 4.0, since it is a serious problem for us too.
Regards.

[gissuebot]

From rfilipov on June 20, 2014 08:02:01

A unit test that exposes the bug and confirms the fix of remi.bantos is attached.

Attachment: gist
   JpaPersistServiceTest.java

[sameb]

Fixed by pull request #820