My only concern is EM.isOpen() call is made for every operation on the EM.
--Gordon
-----Original Message-----
From: Mitesh.Meswani_at_Sun.COM [mailto:Mitesh.Meswani_at_Sun.COM]On Behalf Of Mitesh Meswani
Sent: Wednesday, August 30, 2006 4:48 PM
To: persistence_at_glassfish.dev.java.net
Cc: Tom Ware
Subject: Re: Code review for issue 1003
Gordon Yorke wrote:
Hello Mitesh,
Having the EMF close all of the EMs is a requirement of the specification. That is why this list is here, we could also have the EM's store a reference to the EMF and check the state of the EMF on each operation but this would be expensive, possibly more expensive then maintaining the list.
Following is from javadoc of EMF.close() from the spec
/
Close the factory, releasing any resources that it holds.
* After a factory instance is closed, all methods invoked on
* it will throw an IllegalStateException, except for isOpen,
* which will return false. Once an EntityManagerFactory has
* been closed, all its entity managers are considered to be
* in the closed state.
/
So, we need to ensure that em.isOpen returns false if the emf is closed. If there is no resource leak, I think not maintaining the list might be a more performant option. We can modify em.isOpen() as follows. Please note that em already has a reference to the factory and the method call into emf would be optimized away by hotspot compiler
EMImpl#isOpen() {
return isOpen && factory.isOpen() ;
}
What do you think?
Could we not extend WeakReference and make linked list elements that extend enqueue()? Then as the GC queued the WeakReference it could remove itself from the linked list in the EMF?
Thats the more involved fix I was mentioning yesterday.
Thanks,
Mitesh
--Gordon
-----Original Message-----
From: Mitesh.Meswani_at_Sun.COM [mailto:Mitesh.Meswani_at_Sun.COM]On Behalf Of
Mitesh Meswani
Sent: Wednesday, August 30, 2006 3:14 PM
To: persistence_at_glassfish.dev.java.net
Cc: Gordon Yorke; Tom Ware
Subject: Re: Code review for issue 1003
Scott Oaks wrote:
If there are 1000s of clients accessing the application, does the list
have all the client EMs? Or is there a different factory for each
client? I'm a little concerned that cycling through the list could
become expensive if there are that many simultaneous transactions -- and
then the list becomes its own bottleneck.
The clients referring to the same Persistence Unit will share
EntityManagerFactory.
I'm actually not sure about this entire thing: why can't the em.close()
method remove itself from the list? Then the list wouldn't need to have
WeakReferences at all, and when (if) the factory is closed, it can deal
with the existing EMs.
Scott,
The EMF can be long living object. If an application is not well behaved
(that is it does not close ems) and we have a hard reference to it in
the list, then the ems would be around till the EMF is closed
Gordon,
As I mentioned in the previous mail, I am not sure why the list needs to
be maintained at all. The only consumer of the list is EMF#close() which
calls close() on ems that are still strongly referenced. Do you think we
would leak any resources if an emf is closed but corresponding ems are
not closed? Please note that with current approach, we are still not
calling close() for the ems that the app did not close and were no
longer strongly referenced.
Thanks,
Mitesh
-Scott
On Wed, 2006-08-30 at 14:28, Mitesh Meswani wrote:
Gordon Yorke wrote:
Hello Mitesh,
This should be suffecient. We could move the code to createEntityManager instead of close(). That would ensure that the references are always cleaned up.
Good suggestion. It can help remove the synchronized blocks guarding
entityManagers. However, the list "entityManagers" would keep on growing
till a gc kicks in and we would be iterating the growing list every time
we create an entitymanager. With current approach for a well behaved
application, the list would be trimmed for each close. So, the choice is
between eliminating synchronized blocks or reducing iteration size.
Scott,
Any suggestions on which approach might be better? Attaching the patch
again for your reference
Thanks,
Mitesh
--Gordon
-----Original Message-----
From: Mitesh.Meswani_at_Sun.COM [mailto:Mitesh.Meswani_at_Sun.COM]On Behalf Of
Mitesh Meswani
Sent: Tuesday, August 29, 2006 8:58 PM
To: Gordon Yorke; Tom Ware
Cc: persistence_at_glassfish.dev.java.net
Subject: Code review for issue 1003
Hi Gordon,
Please find attached a fix for issue 1003
<
https://glassfish.dev.java.net/issues/show_bug.cgi?id=1033>.
Following is a summary:
Changed the EntityManagerFactoryImpl.#entityManagers to be a linked list
Added a new method emClosed() to EntityManagerFactoryImpl. This method
is called from EntityManagerImpl.close(). The method iterates over the
list and cleans up as required.
Tests run:
A local test exercising the code.
Scott Oaks is running Specj with this code.
This bug is one of the show stoppers for the UR1 release which has a HCF
tomorrow (Wed). :(
Please note that the fix has a limitation of not cleaning up the list in
EntityManagerFactoryImpl if none of the ems of an application call
close(). The implementation to solve it is a bit more involved. At this
point I am wondering whether we need to maintain the list of
entitymanagers at all in. I can implement the involved fix if
maintaining the list is really required.
Thanks,
Mitesh