code review

Michael Foley mfoley at redhat.com
Wed Jul 27 20:39:03 UTC 2011


Slightly orthogonal ... 

Coverity reports a resource leak with Augeas .... specifically /org/rhq/plugins/apache/augeas/AugeasConfigurationApache.java 

http://coverity.lab.bos.redhat.com:8080/sourcebrowser.htm?projectId=10030#mergedDefectId=13005&streamDefectId=13021&defectInstanceId=143624&fileInstanceId=462202 

A FileInputStream being leaked under some circumstances. This is not the big leak affecting customers, but maybe take a quick look at this one too? 


----- Original Message -----
From: "Ian Springer" <ian.springer at redhat.com> 
To: rhq-devel at lists.fedorahosted.org 
Sent: Wednesday, July 27, 2011 3:09:11 PM 
Subject: Re: code review 

Looks good to me. 

One thing I'd change in all of the try-finally blocks is to catch RuntimeException when calling augeasComponent.close() and log an error rather than letting such an exception bubble up to the PC and causing it to consider the facet call (e.g. loadResourceconfiguration()) to have failed. e.g.: 

} finally { 
if (comp != null) { 

try { 
comp.close(); 
} catch (RuntimeException re) { 
LOG.error("Failed to close Augeas component.", re); 
} 
} 

} 
And one other nit-pick in AugeasRawConfigHelper.java : 

} catch (Exception e) { 
+ if (aug != null) { 
+ try { 
+ aug.close(); 
+ } catch (Exception e2) { 
+ } 
+ aug = null; 
+ } 
+ throw e; 
+ } 

No need to set augeas to null if you're going to throw an exception on the following line. 



On 07/27/2011 11:50 AM, Filip Drabek wrote: 

Hi, 
as you know there is a problem with memory leak in augeas based plugins. ( https://bugzilla.redhat.com/show_bug.cgi?id=721152 ) 
I would like to ask you for code review of my changes in augeas based plugins. The code is in branch release-3.0.0 
commit 4cfd2eb0b6f12bb9e4cd77862a0aa0e2292580e2 and b787f3937f7618bfc83aad8bbc4e4ed67eb97b3e 
The problem was that after initialization of augeas we were calling augeas.load() to load the configuration files, but 
we were not calling method augeas.close() to release the memory in heap. 

The solution is: 
- added method finalize where we can close the augeas. 
- once augeas is initialized and the method load() is called multiple times the memory usage is still growing. 
So we can not reuse the augeas instance and we need to call method close() after each augeas use (started by method load()) 

I have tried more solutions like new augeas components or implementing inner classes for each use of augeas, but in the end 
the code was always much worse than in this implemented solution. 

Thanks 
Filip 






_______________________________________________
rhq-devel mailing list rhq-devel at lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/rhq-devel 

-- 
Ian Springer
Principal Software Developer
JBoss Operations Network
Red Hat ian.springer at redhat.com 
_______________________________________________ 
rhq-devel mailing list 
rhq-devel at lists.fedorahosted.org 
https://fedorahosted.org/mailman/listinfo/rhq-devel 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: https://fedorahosted.org/pipermail/rhq-devel/attachments/20110727/2101cf5e/attachment.html 


More information about the rhq-devel mailing list