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

[GoogleCodeExporter]

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 reported on code.google.com by michal.g...@gmail.com on 31 Jan 2011 at 11:41

[GoogleCodeExporter]

We should probably correct the javadoc.

Original comment by dha...@google.com on 8 Feb 2011 at 4:12

[GoogleCodeExporter]

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?

Original comment by michal.g...@gmail.com on 8 Feb 2011 at 7:46

[GoogleCodeExporter]

Issue 604 has been merged into this issue.

Original comment by sberlin on 18 Feb 2011 at 2:49

[GoogleCodeExporter]

Original comment by sberlin on 22 Feb 2011 at 1:45

• Added labels: Extension-Persist

[GoogleCodeExporter]

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.

Original comment by itscore...@gmail.com on 23 May 2011 at 10:43

[GoogleCodeExporter]

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?

Original comment by g.zoritc...@gmail.com on 26 May 2011 at 12:18

[GoogleCodeExporter]

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-sitebric
ks/

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.

Original comment by remi.bantos on 27 May 2013 at 8:16

[GoogleCodeExporter]

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. 

Original comment by martins....@gmail.com on 28 May 2013 at 7:46

[GoogleCodeExporter]

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.

Original comment by st.clas...@gmx.ch on 28 May 2013 at 8:02

[GoogleCodeExporter]

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.

Original comment by martins....@gmail.com on 28 May 2013 at 8:18

[GoogleCodeExporter]

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?)

Original comment by remi.bantos on 6 Jun 2013 at 1:28

[GoogleCodeExporter]

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()

Original comment by remi.bantos on 13 Jun 2013 at 5:54

[GoogleCodeExporter]

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)

Original comment by twr....@gmail.com on 16 Jul 2013 at 8:21

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

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.)

Original comment by tae...@gmail.com on 16 Oct 2013 at 3:37

[GoogleCodeExporter]

Hi, 

I have implemented a fix for this issue as already mentioned above.
http://code.google.com/r/remibantos-guice-persist/source/detail?r=3a9d1012fedd40
3c2aaddc47701f8ace9eaa3eb8

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.

Original comment by remi.bantos on 16 Oct 2013 at 4:26

[GoogleCodeExporter]

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.)

Original comment by tae...@gmail.com on 16 Oct 2013 at 4:48

[GoogleCodeExporter]

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

• Added labels: Component-Persist
• Removed labels: Extension-Persist

[GoogleCodeExporter]

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.

Original comment by fjackst...@gmail.com on 20 Jun 2014 at 12:05

[GoogleCodeExporter]

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

Original comment by rfili...@gmail.com on 20 Jun 2014 at 3:02

Attachments:

• JpaPersistServiceTest.java

[GoogleCodeExporter]

Pull request is here: https://github.com/google/guice/pull/820

Original comment by fjackst...@gmail.com on 18 Jul 2014 at 1:57

[GoogleCodeExporter]

Original comment by sberlin on 18 Jul 2014 at 2:07

• Changed state: MigratedToGitHub