Issue 597 in google-guice: Persist Extension: UnitOfWork.begin() throws IllegalStateException when called multiple times

[google...@googlecode.com]

Status: New
Owner: ----

New issue 597 by michal.g...@gmail.com: Persist Extension:
UnitOfWork.begin() throws IllegalStateException when called multiple times
http://code.google.com/p/google-guice/issues/detail?id=597

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?

[google...@googlecode.com]

Comment #1 on issue 597 by dha...@google.com: Persist Extension:

UnitOfWork.begin() throws IllegalStateException when called multiple times
http://code.google.com/p/google-guice/issues/detail?id=597

We should probably correct the javadoc.

[google...@googlecode.com]

Comment #2 on issue 597 by michal.g...@gmail.com: Persist Extension:

UnitOfWork.begin() throws IllegalStateException when called multiple times
http://code.google.com/p/google-guice/issues/detail?id=597

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?

[google...@googlecode.com]

Comment #3 on issue 597 by sberlin: Persist Extension: UnitOfWork.begin()

throws IllegalStateException when called multiple times
http://code.google.com/p/google-guice/issues/detail?id=597

Issue 604 has been merged into this issue.

[google...@googlecode.com]

Updates:
Labels: Extension-Persist

Comment #4 on issue 597 by sberlin: Persist Extension: UnitOfWork.begin()

throws IllegalStateException when called multiple times
http://code.google.com/p/google-guice/issues/detail?id=597

(No comment was entered for this change.)

[google...@googlecode.com]

Comment #5 on issue 597 by i...@it-score.de: Persist Extension:

UnitOfWork.begin() throws IllegalStateException when called multiple times
http://code.google.com/p/google-guice/issues/detail?id=597

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.

[google...@googlecode.com]

Comment #6 on issue 597 by g.zoritc...@gmail.com: Persist Extension:

UnitOfWork.begin() throws IllegalStateException when called multiple times
http://code.google.com/p/google-guice/issues/detail?id=597

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?

[google...@googlecode.com]

Comment #7 on issue 597 by remi.ban...@gmail.com: Persist Extension:

UnitOfWork.begin() throws IllegalStateException when called multiple times
http://code.google.com/p/google-guice/issues/detail?id=597

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.



--
You received this message because this project is configured to send all
issue notifications to this address.
You may adjust your notification preferences at:
https://code.google.com/hosting/settings

[google...@googlecode.com]

Comment #8 on issue 597 by martins....@gmail.com: Persist Extension:

UnitOfWork.begin() throws IllegalStateException when called multiple times
http://code.google.com/p/google-guice/issues/detail?id=597

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.

[google...@googlecode.com]

Comment #9 on issue 597 by st.clas...@gmx.ch: Persist Extension:

UnitOfWork.begin() throws IllegalStateException when called multiple times
http://code.google.com/p/google-guice/issues/detail?id=597

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.

[google...@googlecode.com]

Comment #10 on issue 597 by martins....@gmail.com: Persist Extension:

UnitOfWork.begin() throws IllegalStateException when called multiple times
http://code.google.com/p/google-guice/issues/detail?id=597

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.

[google...@googlecode.com]

Comment #11 on issue 597 by remi.ban...@gmail.com: Persist Extension:

UnitOfWork.begin() throws IllegalStateException when called multiple times
http://code.google.com/p/google-guice/issues/detail?id=597

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

[google...@googlecode.com]

Comment #12 on issue 597 by remi.ban...@gmail.com: Persist Extension:

UnitOfWork.begin() throws IllegalStateException when called multiple times
http://code.google.com/p/google-guice/issues/detail?id=597

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

[google...@googlecode.com]

Comment #13 on issue 597 by twr....@gmail.com: Persist Extension:

UnitOfWork.begin() throws IllegalStateException when called multiple times
http://code.google.com/p/google-guice/issues/detail?id=597

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)

[google...@googlecode.com]

Comment #14 on issue 597 by remi.ban...@gmail.com: Persist Extension:

UnitOfWork.begin() throws IllegalStateException when called multiple times
http://code.google.com/p/google-guice/issues/detail?id=597

I'm a little bit pessimistic for this fix to be released by someone soon...
It seems like there is no more activity nor maintenance on guice-persist
(and even guice?). Just take a look at open / not analysed issues.

After having pushed on google code this fix (to a fork), i had to bring
myself to fix it...by myself. So i did it by customysing the
JpaPersistService on my webapp ... :(

By the way, there are a lot of duplicates or similar issues on
guice-persist, so there would probably be another solutions to deal with
this problem.

Injection of several data-sources with guice-persist is quite painfull.
So I plan to migrate my app to a CDI implementation soon and use EJB
persistence unit injection (which is very easy to do for dealing with many
datasources), perhaps guice owners plan the same :)

(Seriously, it would be great for users to have a clear view on guice
roadmap if there is one.)

[google...@googlecode.com]

Comment #15 on issue 597 by remi.ban...@gmail.com: Persist Extension:

UnitOfWork.begin() throws IllegalStateException when called multiple times
http://code.google.com/p/google-guice/issues/detail?id=597

I'm a little bit pessimistic for this fix to be released by someone soon...

It seems like there is no more activity nor maintenance on guice-persist.

Just take a look at open / not analysed issues.

After having pushed on google code this fix (to a fork), i had to bring
myself to fix it...by myself. So i did it by customysing the
JpaPersistService on my webapp ... :(

By the way, there are a lot of duplicates or similar issues on
guice-persist, so there would probably be another solutions to deal with
this problem.

[google...@googlecode.com]

Comment #16 on issue 597 by tae...@gmail.com: Persist Extension:

UnitOfWork.begin() throws IllegalStateException when called multiple times
http://code.google.com/p/google-guice/issues/detail?id=597

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

[google...@googlecode.com]

Comment #17 on issue 597 by remi.ban...@gmail.com: Persist Extension:

UnitOfWork.begin() throws IllegalStateException when called multiple times
http://code.google.com/p/google-guice/issues/detail?id=597

Hi,

I have implemented a fix for this issue as already mentioned above.
http://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.

[google...@googlecode.com]

Comment #18 on issue 597 by tae...@gmail.com: Persist Extension:

UnitOfWork.begin() throws IllegalStateException when called multiple times
http://code.google.com/p/google-guice/issues/detail?id=597

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

[google...@googlecode.com]

Updates:
Labels: -Extension-Persist Component-Persist

Comment #19 on issue 597 by sberlin: Persist Extension: UnitOfWork.begin()

throws IllegalStateException when called multiple times
http://code.google.com/p/google-guice/issues/detail?id=597

(No comment was entered for this change.)

[google...@googlecode.com]

Comment #20 on issue 597 by fjack...@gmail.com: Persist Extension:

UnitOfWork.begin() throws IllegalStateException when called multiple times
http://code.google.com/p/google-guice/issues/detail?id=597

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.

[google...@googlecode.com]

Comment #21 on issue 597 by rfili...@gmail.com: Persist Extension:

UnitOfWork.begin() throws IllegalStateException when called multiple times
http://code.google.com/p/google-guice/issues/detail?id=597

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



Attachments:
JpaPersistServiceTest.java 2.2 KB

[google...@googlecode.com]

Comment #22 on issue 597 by fjackst...@gmail.com: Persist Extension:

UnitOfWork.begin() throws IllegalStateException when called multiple times
http://code.google.com/p/google-guice/issues/detail?id=597

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

[google...@googlecode.com]

Updates:
Status: MigratedToGitHub

Comment #23 on issue 597 by sberlin: Persist Extension: UnitOfWork.begin()

throws IllegalStateException when called multiple times
http://code.google.com/p/google-guice/issues/detail?id=597

(No comment was entered for this change.)