Re: Code review for issue 1003
Scott,
What do you think?
Thanks,
Mitesh
Gordon Yorke wrote:
> 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
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>