Automatically started UnitOfWork is never ended

[GoogleCodeExporter]

We've been using Guice in our projects for a while now, and it's been great. 
There's one feature/bug that is a reliable source of bugs for us, though.

When using the JpaPersistService, if you attempt to access an EntityManager 
outside of an active UnitOfWork, Guice will automatically start one for you. 
However, as Guice does not (and cannot) know when to end this UnitOfWork, it 
never does.

Result? The offending thread will be stuck with the same EntityManager 
throughout the life of the application. This is a bad state for the application 
to run in, and ours inevitably exhaust the available memory after a while and 
crash.

The real killer here is that it's not at all obvious when you've made this 
mistake. The only real tip-off is that you're getting inconsistent data from 
your database between different threads (due to the EMs first-level cache) or 
that the applications memory consumption keeps on growing.

I think it would be much better behaviour to simply throw an exception if an 
attempt is made to access an EntityManager instance outside of an active 
UnitOfWork. This lets the developer know that they've made a mistake and are 
attempting to access the database outside of a valid scope.

Steps to reproduce:
1. 'mvn test' on the supplied maven project (assumes postgres running on 
localhost:5432 with DB/user/pass 'test')

Original issue reported on code.google.com by spootsy....@gmail.com on 27 Sep 2012 at 2:48

Attachments:

• guice_persist_same_em.zip

[GoogleCodeExporter]

I've created a git patch which addresses this issue, as well as issue 731. 
Rundown of changes from the commit log:
- JpaPersistService now counts invocations of begin() and end(), and only closes
  EntityManager when end() has been called an appropriate number of times
- Attempting to get an EntityManager outside of an active UnitOfWork now results
  in an IllegalStateException
- Added a new @WorkUnit annotation. This is equivalent to the @Transactional
  annotation for methods/classes which access the database but do not modify it

Patch also updates the appropriate unit tests.

Original comment by spootsy....@gmail.com on 10 Oct 2012 at 11:44

Attachments:

• 0001-Updates-to-JpaPersistService-implementation-of-UnitO.patch

[GoogleCodeExporter]

@WorkUnit annotation, is this similar to Spring's @Transactional(readOnly = 
true)
http://static.springsource.org/spring/docs/3.1.x/spring-framework-reference/html
/transaction.html#transaction-declarative-annotations
http://static.springsource.org/spring/docs/3.1.x/javadoc-api/org/springframework
/transaction/annotation/Transactional.html#readOnly()

IMO, readOnly=true expresses the transaction's behavior clearly that the 
changes will not be committed. 

Original comment by prashant...@gmail.com on 11 Oct 2012 at 1:54

[GoogleCodeExporter]

@WorkUnit is just a shorthand means of using the UnitOfWork interface, by 
wrapping the annotated method with calls to uow.begin/uow.end in a try/finally 
block. No JPA transaction is explicitly started.

I think it's conceptually different from a read-only transaction. That would 
certainly make more sense as an additional attribute on the existing 
@Transactional annotation.

Original comment by spootsy....@gmail.com on 11 Oct 2012 at 3:11

[GoogleCodeExporter]

Will this fix be part of Guice 4.0?

Original comment by bimsc...@gmail.com on 22 Aug 2013 at 9:38

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

I was hoping they'd include a patch of some kind but I don't see any changes in 
trunk. It's a shame, because the current semantics of UnitOfWork are pretty 
badly broken IMHO.

Also, please note there's a significant bug in this patch, as discussed here: 
http://code.google.com/p/google-guice/issues/detail?id=731 

It's a simple fix but I haven't got around to it yet. If anybody else has a 
vested interest you're more than welcome to fix it and post the updated patch 
here.

Original comment by spootsy....@gmail.com on 23 Aug 2013 at 5:01

[GoogleCodeExporter]

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

• Added labels: Component-Persist