[#UP-992] MultipartDataSource.finalize( ) is not implemented properly.

uPortal

MultipartDataSource.finalize( ) is not implemented properly.

Created: 22/Apr/05 10:48 AM Updated: 03/Jul/07 04:11 PM

Component/s:

Framework

Affects Version/s:

2.3.5, 2.4, 2.4.1, 2.4.2

Fix Version/s:

2.4.3

Original Estimate:

Unknown

Remaining Estimate:

Unknown

Time Spent:

Unknown

File Attachments:

None

Image Attachments:

1. MultiPartDataSource_GCRoot.jpg
(224 kb)

Issue Links:

Duplicate

This issue is duplicated by:
	~~UP-1177~~ org.jasig.portal.MultipartDataSource ...

Estimated End Date:

22/Apr/05

Description

« Hide

MultipartDataSource.finalize( ) is not implemented properly.

I see two problems with this implementation

1- The method signature is little strange to me. Shouldn't it be?

protected void finalize() throws throwable

1- Adding a finalize method will delay cleaning up the memory..surprising ,but it's true.. now that's becoz GC maintains a separate queue for objects that have finalize method( finalization queue).. so ultimatley it requires 2 GCs to free up memory, thus inceasing load on memory.

I suggest we should get rid of finalize and add a new method as follows

protected void dispose() {
    buff = null;
    if(tempfile!= null){
        tempfile.delete();
        tempfile = null;
    }
  }

All

Comments

Work Log

Change History

Sort Order:

[ Permlink | « Hide ]

Andrew Petro [22/Apr/05 11:49 AM]

Agreed that we should add a dispose() method and declare in the API for this object that clients of this object should call dispose() when done with it, so it can clean up its temp file.

Do we still need to implement the finalizer to call dispose() so that in the case where our client failed to call dispose() and just dropped its reference to the MultipartDataSource instance, we will clean up the file system resources that we were using? Calling dispose() in the finalizer may be necessary to guarantee cleanup of external resources?

[ Permlink | « Hide ]

Faizan Ahmed [22/Apr/05 01:54 PM]

As I mentioned in the issue that Adding a finalize method will delay cleaning up the memory. I know it sounds good to provide finalizer as a safety net in case the client of this class fail to call dispose method explictly. I know Bill will say no finalizer

Anyway I will keep finalizer for now. Following is the code that I am about to commit.

protected void finalize() throws Throwable {
try { dispose(); }finally { super.finalize(); }
}

/**

Releases tempfile associated with this object any memory
they consume will be returned to the OS.
*
*/
public void dispose()
Unknown macro: { buff = null; if (tempfile != null) { tempfile.delete(); tempfile = null; } }

[ Permlink | « Hide ]

Faizan Ahmed [22/Apr/05 02:44 PM]

-dispose( ) method is added to delete the temp files associated with this
object.
-The finalizer is now properly implemented and it calls dispose() method.
finalizer should never be used to release resources since finalizer are
unpredictable.

Updated the class comment to mention that clients should call dispose to
cleanup.

[ Permlink | « Hide ]

Faizan Ahmed [28/Apr/05 02:10 PM]

I just took snapshot from our production system and found few MultiPartDataSource Objects
held in memory blaming the finalizer.
I am going to remove the finalizer from this class.

[ Permlink | « Hide ]

Faizan Ahmed [22/Jul/05 10:23 AM]

I created this issue by mistake. I thought I forgot to add it in JASIG jira. Sorry for inconvenience.