code review

Lukas Krejci lkrejci at redhat.com
Thu Jul 28 07:36:22 UTC 2011


Well, this has nothing to do with BZ 721152 which deals with a memory leak of 
native memory held by the augeas native library data structures that we didn't 
properly close.

The resource leak you mention is purely originating in java - we just don't 
close a file we're reading. That of course is a bug but, as you mention, a 
different one from the one being discussed in this thread.

On Wednesday, July 27, 2011 22:39:03 Michael Foley wrote:
> 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#m
> ergedDefectId=13005&streamDefectId=13021&defectInstanceId=143624&fileInstan
> ceId=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


More information about the rhq-devel mailing list