Memory Leak in a multi threaded Swing application (ImageIcon)
When I profile my swing application with netbeans 5.5 , I notice that after each periodically tidy up my image container (add,remove IconImage objects to a hashmap or arraylist), there gather by and bysurviving objects. This leads to run out of memory after a while. Is there any other way to avoid this effect?
Any ideas about "The JVM is notorious for caching images."
please help..
what I have made briefly:
1.) Read the binary stream from the db :
rs=stmt.executeQuery(query);
if(rs.next()){
int len=rs.getInt(2);
byte [] b=newbyte[len];
InputStream in = rs.getBinaryStream(3);
try{
in.read(b);
in.close();
img=Toolkit.getDefaultToolkit().createImage(b);
}catch (IOException e){
e.printStackTrace();
}
}
stmt.close();
rs.close();
2.) hold the icon as field :
this.icon =new ImageIcon(img);
3.) After a while I remove the object from my collection and on the
overridden method finalize() I also call the flush() method.
if(this.icon !=null){
this.icon.getImage().flush();
this.icon =null;
}
The surviving objects still increase?! On the page of SUN they
submitted a bug. But this is set on closed/fixed.
(http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4014323)
What am I doing wrong? I also use the byte[] constructor on creating the icon and
my java version is 1.5.0_10.
[2074 byte] By [
kapkara@a] at [2007-11-26 14:37:53]
# 1
This should not make a difference, but I'd close the ResultSet, before the statement.According to the JDBC specs, closing the the statement should close the result set, but you never know.
# 2
yes, I checked also all streams. These are properly closed. The worst thing is I have no chance to cache objects (Forms, dialogs etc.) in my collections which contains icons. I guess this issue is related with Icon objects. Or other swing objects? really despairing!
# 3
Caches should use Weak/SoftReferences.
# 4
InputStream in = rs.getBinaryStream(3);
try {
in.read(b);
in.close();
You cant read an input stream that way. But that is not the reason for memory leak.
Check on which places that you hold the references to the objects.
Check whether the objects are really getting removed when you remove from collection.
LRMKa at 2007-7-8 8:18:56 >
# 5
I stripped-down my code, this also gathers surviving objects. An application containing this code could not live long.
public class LeakCheck {
private int counter=0;
private final String randomquery = "SELECT TOP 4 T00801PersonNr AS PersonNr," +
" T00801PersonID AS PersonID, T00801Nachname AS Nachname, " +
" T00801Vorname AS Vorname FROM DBA.T00801Person ORDER BY RAND()";
private class Person extends Object {
private javax.swing.ImageIcon icon;
private String number;
protected void finalize() throws Throwable {
if(icon!=null){
icon.getImage().flush();
icon = null;
}
super.finalize();
}
}
private ArrayList<Person> personList = new ArrayList<Person>();
protected class RefreshTask extends TimerTask {
@Override
public void run() {
random();
}
}
public void go(){
Timer updateTimer = new Timer("Refresh Task");
updateTimer.schedule(new RefreshTask(),0,2000);//instance,delay,interval
}
private Image LoadImageFromDB(String personnr){
Image img = null;
String filename = personnr + ".jpg";
Connection con = DbInterface.getinstance().getEtrainerDBConnection();
Statement stmt;
ResultSet rs;
try {
stmt = con.createStatement();
String query = Resources.getInstance().getDbProperties().getProperty("selectPersonImage");
query = String.format(query, filename);
rs=stmt.executeQuery(query);
if(rs.next()){
int len=rs.getInt(2);
byte [] b=new byte[len];
InputStream in = rs.getBinaryStream(3);
try {
in.read(b);
in.close();
img=Toolkit.getDefaultToolkit().createImage(b);
} catch (IOException e) {
e.printStackTrace();
}
}
rs.close();
stmt.close();
} catch (SQLException e) {
e.printStackTrace();
}
return img;
}
public void random(){
java.sql.Connection con = DbInterface.getinstance().getSybaseDBConnection();
try {
if(con!=null && !con.isClosed()){
java.sql.Statement stmt=null;
try {
stmt = con.createStatement();
java.sql.ResultSet rs = stmt.executeQuery(randomquery);
while(rs.next()){
Person person = new Person();
person.number = rs.getString("PersonNr");
Image img = LoadImageFromDB(person.number);
if(img !=null){
ImageIcon ico = new ImageIcon(img);
person.icon = ico;
}
personList.add(person);
System.out.println("Container size: " + personList.size());
counter++;
}
if(counter%20 == 0){
personList.clear();
System.gc();//no need, but I force for this example
System.out.println("Container cleared, size: " + personList.size());
}
rs.close();
stmt.close();
} catch (SQLException ex) {
ex.printStackTrace();
}
}
} catch (SQLException ex) {
ex.printStackTrace();
}
}
public static void main(String[] args) {
LeakCheck leakCheck = new LeakCheck();
leakCheck.go();
}
}
# 6
Well, I try to give you two suggestion
1) try this first
In your Person class put this metod
public void destroy()
{
icon.getImage().flush();
icon = null;
}
And delete the finalize() method; remember that this method is call only if your object is "garbageable" but in your cases seems not....
Before invoking personList.clear() method from random() method,
on each object contained in your personList, invoke the detroy method
and after invoke personList.clear()
2) Only if the first suggestion does not works.....
ImageIcon object loads images using a protected field named "tracker"
If u see loadImage method, it uses "tracker" instance to load an image
So, u can extend ImageIcon class in your own "MyImageIconClass"
and put into it a your own removeImage() that performs something like
this:
public void removeImage(Image anImage)
{
tracker.removeImage(anImage);
}
So, your person Object instead of storing an ImageIcon reference
has to store a MyImageIcon reference and your destroy() method
must invoke MyImageIcon.removeImage() method
I have take a breef look of MediaTracker addImage method.
It adds the image to a MediaEntry Object that has a MediaTracker reference itself (...oh,... my God......)
I suppose that explictly removing the images from the tracker, your objects will be promoted to "garbageable"
I hope this may help you
Good Luck
silsa at 2007-7-8 8:18:56 >
# 7
Even better, this should work much faster:JPEGImageDecoder decoder = JPEGCodec.createJPEGDecoder(in);image = decoder.decodeAsBufferedImage();
# 8
One simple question: have you established which objects are being retained? Are they images, database connections, or what?
Also, you're not showing all your code. It's not possible to see what your DbInterface class is doing, for example.
If you rewrite your code so as not to use your database (you can use MemoryImageSource to create images from int arrays) then we can run it and test it. As it is we can't - without seeing all the code and without being able to run it there's only so much help you can get.
I will say (though this won't be causing your leak) that your database closing code is flawed - if an SQLException occurs you don't close resources; and don't forget that the close() methods themselves may throw SQLException. The same goes for your stream reading code - if your read() throws an exception then you don't close() the stream, and again don't forget that close() can throw as well. So in both cases, any exceptions will almost certainly cause a leak.
Suffice it to say that creating and destroying images and caching them works absolutely fine, I do it all the time, so the bug is in your code, not ImageIcon.
# 9
1) I changed the database connection. Now it is not constantly open. Each time on demand the connection is opened and closed.
2) I implemented also a wrapper around the imageicon. And destroy it as suggested.
private class MyImageIcon extends ImageIcon {
public MyImageIcon(Image img){
super(img);
}
public void removeImage(Image anImage){
tracker.removeImage(anImage);
}
}
Now the image leak is gone! thx.
But if I go one step further and put in my collection also a form object (a very simple JForm extended object) the leak appears again. I have made a dispose() call but the surviving objects increase again.
Here a link to two java files and two snapshots (if providing a link is not a good idea, tell me, I will then post the complete class code).
http://www.box.net/public/kf5a0q5bqh
# 10
> w the image leak is gone! thx.Thx you for the dukes.......If in your JFrame u have placed your image, before invoke the dispose()method put explicitly the image pointer to null and deregister all listeneryou have added to any componentRegards
silsa at 2007-7-8 8:18:56 >
# 11
> If in your JFrame u have placed your image, before invoke the dispose()
> method put explicitly the image pointer to null and deregister all listener
> you have added to any component
I implemented your suggest and after starting a long time test, in one hour there gathered aprox. 500 surviving generations. I attach a snapshot and the java file ( http://www.box.net/public/eqznamrazd ). The used heap size swings between 3MB and 5MB. I guess this wont kill so quickly the application but anyway there is something wrong!
Even properly closed streams, database connections etc.. Really despairing. Could one take a look to the java source?
some snippets bellow:
private class MyImageIcon extends ImageIcon {
public MyImageIcon(Image img){
super(img);
}
public void removeImage(Image anImage){
tracker.removeImage(anImage);
}
}
private class DetailDialog extends javax.swing.JFrame {
private String personnr;
private MyImageIcon icon;
public DetailDialog(String personnr,MyImageIcon icon){
this.personnr = personnr;
this.icon = icon;
}
public void dispose() {
if(icon != null){
icon.removeImage(icon.getImage());
icon.getImage().flush();
icon = null;
}
super.dispose();
}
}
private class Person extends Object {
private MyImageIcon icon;
private String number;
private DetailDialog detailDialog;
protected void destroy() {
if(icon!=null){
icon.removeImage(icon.getImage());
icon.getImage().flush();
icon = null;
}
if(detailDialog!=null){
detailDialog.dispose();
}
}
}
private Image LoadImageFromDB(String personnr){
Image img = null;
String filename = personnr + ".jpg";
Connection con = getMysqlConnection();
Statement stmt;
ResultSet rs;
try {
stmt = con.createStatement();
String query = "select * from personImage where image='"+filename+"'";
rs=stmt.executeQuery(query);
if(rs.next()){
int len=rs.getInt(2);
byte [] b=new byte[len];
InputStream in = rs.getBinaryStream(3);
try {
in.read(b);
in.close();
img =
java.awt.Toolkit.getDefaultToolkit().createImage(b);
} catch (IOException e) {
e.printStackTrace();
}
}
rs.close();
rs = null;
stmt.close();
stmt = null;
con.close();
con = null;
} catch (SQLException e) {
e.printStackTrace();
}
return img;
}
public void random(){
java.sql.ResultSet rs = null;
java.sql.Statement stmt=null;
java.sql.Connection con = getSybaseConnection();
try {
try {
stmt = con.createStatement();
rs = stmt.executeQuery(randomquery);
while(rs.next()){
Person person = new Person();
person.number = rs.getString("PersonNr");
Image img = LoadImageFromDB(person.number);
if(img !=null){
MyImageIcon ico = new MyImageIcon(img);
person.icon = ico;
}
person.detailDialog = new
DetailDialog(person.number,person.icon);
personList.add(person);
System.out.println("Container size: " +
personList.size());
counter++;
}
if(counter%20 == 0){
for(Person p : personList){
p.destroy();
}
personList.clear();
System.gc();//no need, but I force for this example
System.out.println("Container cleared, size: " +
personList.size());
}
} catch (SQLException ex) {
ex.printStackTrace();
}finally{
if(rs != null){
rs.close();
}if(stmt != null){
stmt.close();
}if(con != null){
con.close();
}
}
} catch (SQLException ex) {
ex.printStackTrace();
}
}
# 12
Your JPEG is pretty much illegible.
The used heap size swings between 3MB and 5MB.
Do you mean it's not continously growing? Then surely you have nothing to worry about?
Even properly closed streams, database connections etc
You're still not properly closing streams or connections them if an exception occurs. Again, if reading your stream throws then it is not closed. If closing the ResultSet throws then the Connection is not closed. And so on,
Your business of resetting references to null is futile as well - they're discarded when they're out of scope anyway. You're doing a number of things which are just random attempts to call things that look like they should free up resources but which aren't required - for example, ImageIcon is perfectly capable of flushing its image, you don't need to do it explicitly.
I'll go back to my previous point, which is that you haven't separated your database functionality from your image caching. How do you expect to find the problem when you can't distill the code to find it?
Split the two - write a bit of code to just read stuff from the database and a bit of code just to cache images created from int arrays. Then you'll find your actual problem and you'll be able to write an effective algorithm for each, pluggin in the other later.
# 13
> The used heap size swings between 3MB and 5MB. I guess
> this wont kill so quickly the application but anyway
> there is something wrong!
Not now, in my opinion not! I have readed your code. I do not see
any problem, now.
If after 1 hour the heap size swings between 3MB and 5MB in my
opinion that means that is there no memory leaks in your application, and all does well.
I have readed your code.
if u want to help much more the garbage collector u cant try something
like this:
private class Person extends Object {
private MyImageIcon icon;
private String number;
private DetailDialog detailDialog;
protected void destroy() {
if(icon!=null){
icon.removeImage(icon.getImage());
icon.getImage().flush();
icon = null;
}
if(detailDialog!=null){
detailDialog.dispose();
detailDialog = null;
}
}
}
Putting explicitly the detailDialog pointer to null helps the garbage
collector to know that this object can be "garbaged" without warry!
I like to give u another suggestion:
Garbage collector is pretty complicated, but i suggest u to read carefully again ang again this document:
http://java.sun.com/docs/hotspot/gc5.0/gc_tuning_5.html
After this u can use a profiler (Netbeans for example ships a pretty good
profiler for free) for profile the memory allocation of your application.
Attention: is pretty easy to know if is there a memory leak: heap size continuslly grows and the instances number in memory for some classes becames larger and larger.
But, in some situations, may be not so easy to find exactly where the real problem is.
But knowing how the garbage collector works and learning a good profiler will help you to write a robust and performance java application.
Best regard.
silsa at 2007-7-8 8:18:56 >
# 14
I do not see any problem, now.
See above, leaks can occur if any exceptions are thrown.
Putting explicitly the detailDialog pointer to null helps the garbage
collector to know that this object can be "garbaged" without warry!
If the Person object is collectable then this makes no difference. And if the Person object is not collectable, but should be, then you are just reducing the size of the leak rather than eliminating the leak - it's not a proper solution.
There is absolutely no reason to use a destroy() method in all but the rarest of cases (I'm struggling to recall a time in 10 years of writing Java code when I've needed to do it), namely where persistently opened external resources need to be freed (eg services shut down, streams closed etc) - more often than not these are only required transiently so should be shut down at the point of use anyway.
As an aside I'd suggest that a Person retaining a reference to a dialog is a poor design - dialogs are inherently transient and should usually be disposed of when closed.
# 15
> There is absolutely no reason to use a destroy()
> method in all........
I'm not agree with u.
Sun itself says that putting explictly pointers to null helps garbage collector to works better (performances)
In very complex system (it seems to me that is not the case of our friend) may be help. I do not want to say that this solves memory leak problem, but only that, if in code there is not a memory leak, this may
help garbage collector to works better.
> As an aside I'd suggest that a Person retaining a
> reference to a dialog is a poor design - dialogs are
> inherently transient and should usually be disposed
> of when closed.
Yes, when the dialog is closed absolutely yes!
But, about the design: Imagine that our friend needs to store the dialog pointer when dialogs are still opened to do something (for example a Dialog list for allow the application user to close all Dialogs still opened with a pop-up that says "close all")
In this situation, when user closes one Dialog (or all Dialogs using the pop-up), our friend needs to remove first the Dialog strong reference stored in the collection and after he has to dispose the dialog that user has closed.
silsa at 2007-7-21 16:14:22 >
# 16
Sun itself says that putting explictly pointers to null helps garbage collector to works better (performances)
All it does is reduce the granularity of the reference tree. It has no effect whatsoever on whether there is or is not a leak. The main point here is to avoid the misconception that novices sometimes acquire, which is that by nulling a reference the object directly becomes eligible for collection. Setting intra-method references to null is totally irrelevant in terms of memory leaks.
Imagine that our friend needs to store the dialog pointer when dialogs are still opened to do something
Point taken, though two points to counter it:
- most dialogs should be modal; otherwise frames are more appropriate, non-modal dialogs are problematic
- business objects are not the appropriate place for managing closure of UI resources
The dialog issue is unrelated to the memory leak, though. My main point is that suggesting that setting method-local references to null will fix a memory leak is wrong.
# 17
I seperated the concerns and made some little tests
1. person object contains only one icon (created by byte array) and a
string. The collection updates each 3 sec. puts and removes. No database
connection.
2. person object contains one icon (created by byte array) a string and
a JForm object. The collection updates each 3 sec. puts and removes.
No database connection.
3. person object contains a string. The collection updates each
3 sec. puts and removes. Database connection established for query
the string.
Result : no growing heap space but increasing live objects (top of
them are especially java.util.HashMap$Entry, java.util.Hashtable$Entry,
java.util.HashMap$Entry[], java.util.Hashtable$Entry)
The more objects involves into this process gathering object count
increases very high and rapidly. This point frightened me.
I use the netbeans profiler. Referencing a dialog to a person is not a poor
design. On demand the application holds a related form to reduce the
loading time.
# 18
So let's come back to another point I made above. Unless you can post a self-contained executable class which displays a leak then we can't help find your bug.
# 19
Ok, if you already user Netbeans profiler you know that you can force garbage collector execution during the program run by click a toolbar button.
Profile your application and when u see that live object increases, click
that button.
If is there some memory leak (I don't beleave that) the leave object continues to grow, ele if no memory leak is there (as i think) you will
see your object instance counters immediatly decreased.
Let me know.
silsa at 2007-7-21 16:14:22 >
# 20
For a better understanding I have to read carefully the spec. of garbage collector. Surviving generations is one point. But at now the used heap size is under control. thx.
# 21
I also used the gc. button. But even of triggering manually the surviving objects are not under control. Grow constantly.
# 22
>Sun itself says that putting explictly pointers to
> null helps garbage collector to works better
> (performances)
I totally and emphatically disagree. Can you provide some reference
to that effect? I'm pretty **** well-read, and I've never, ever heard
anyone say that it's a good idea.
For instance,
http://www-128.ibm.com/developerworks/java/library/j-perf08273.html
Scroll down to "Best Practice". Proper scoping is correct. Explicit
nulling is incorrect.
# 23
http://developers.sun.com/techtopics/mobility/midp/articles/garbage/index.html#9.1.3Read 9.1.3.5Regards
silsa at 2007-7-21 16:14:22 >
# 24
I think you're taking that out of context.
The key things to bear in mind are scope and lifecycle. There is a world of difference between class fields, instance fields and method variables.
References stored in class fields are maintained until the class is disposed of - this typically occurs when the VM closes, so class fields really should be nulled when no longer required. Collections stored by classes are one of the prime danger areas for leaks.
References stored in instance fields follow the object - so for as long as you have an ImageIcon you have its Image, but as soon as there is no reference to the ImageIcon then the Image is collectable as well. The only purpose of breaking this link is if you somehow wanted an ImageIcon with no Image (which you don't). However, if you have an object with a long lifecycle which has temporary need for field references to other objects, then nulling these fields makes sense.
References in method variables only have a lifecycle of the method's execution - rather, of the braced block they're in. As soon as they move out of scope the reference is lost. So there is no point - ever (maybe there's an extreme example but I can't think of one and you absolutely won't encounter one in normal circumstances) - in nulling these, you're not saving anything.
Understanding scope is fundamental to writing good code in a garbage collected system. Nulling stuff all over the place is not.
# 25
"The easiest is to find objects that are kept alive longer than
necessary, and to stop referring to them as soon as they are no longer
needed. Sometimes, this is as simple as setting the last reference to
null as soon as the object is no longer needed."
Nowhere does that say "this is a good idea". Merely "this is
possible." Lots of things are possible and still bad ideas.
Josh Bloch, Effective Java, Item 5:
"When programmers are first stung by a problem like this, they tend
to overcompensate by nulling out every object reference as soon as
the program is finished with it. This is neither necessary nor
desirable as it clutters up the program unnecessarily and could
conceivably reduce performance. Nulling out object references should
be the exception rather than the norm. The best way to eliminate
an obsolete reference is to reuse the variable in which it was
contained or let it fall out of scope."
