ng4j-namedgraphs

[namedgraphs] Memory leak in database persistence implementation

[Graeme S. <ac...@gm...>]

Hi guys,

Firstly, thanks for all your hard work on the NG4J library. I've found it
extremely useful over the past couple of years.

I've found a memory leak in the database persistence implementation in
version 0.9.3 caused by ResultSet objects not being closed when a (positive)
check is made for the existence of a Quad. The following conditions
reproduce the bug:

- A call is made to QuadDB.find(..) to check the existence of a Quad
- The hasNext() call on the returned iterator returns true
- No further methods are invoked on the iterator

The implementation of the QuadDB.find(..) currently only closes the
ResultSet object when hasNext() returns false, or an exception occurs.

There are two methods that call QuadDB.find(..) where this will always occur
when a match is found:
QuadDB.insert(...)
NamedGraphSetDB.containsQuads(...)

And this could potentially occur in the following methods if the iterator is
not completely traversed:
NamedGraphSetDB.findQuads(...)
NamedGraphDB.graphBaseFind(...)

Hope this is of use to you,

Cheers,
Graeme.

Re: [namedgraphs] Memory leak in database persistence implementation

[Jennifer C. <jen...@at...>]

Good catch, Graeme!

 

Has anyone looked into this?

 

I think we'll want to have the find method return some sort of closeable
iterator, such as com.hp.hpl.jena.util.iterator.ClosableIterator, instead
of java.util.Iterator, and then the caller should call close if the
iterator is not fully traversed.  Similarly for method listGraphNames in
the same class (QuadDB).

 

I may try that and see .

 

Jennifer

 

 

From: Graeme Stevenson [mailto:ac...@gm...] 
Sent: Thursday, July 15, 2010 8:20 AM
To: ng4j-namedgraphs
Subject: [namedgraphs] Memory leak in database persistence implementation

 

Hi guys,

Firstly, thanks for all your hard work on the NG4J library. I've found it
extremely useful over the past couple of years. 

I've found a memory leak in the database persistence implementation in
version 0.9.3 caused by ResultSet objects not being closed when a
(positive) check is made for the existence of a Quad. The following
conditions reproduce the bug:

- A call is made to QuadDB.find(..) to check the existence of a Quad
- The hasNext() call on the returned iterator returns true
- No further methods are invoked on the iterator

The implementation of the QuadDB.find(..) currently only closes the
ResultSet object when hasNext() returns false, or an exception occurs.

There are two methods that call QuadDB.find(..) where this will always
occur when a match is found:
QuadDB.insert(...)
NamedGraphSetDB.containsQuads(...)

And this could potentially occur in the following methods if the iterator
is not completely traversed:
NamedGraphSetDB.findQuads(...)
NamedGraphDB.graphBaseFind(...)

Hope this is of use to you,

Cheers,
Graeme.

Re: [namedgraphs] Memory leak in database persistence implementation

[Jennifer C. <jen...@at...>]

I clearly spoke too quickly, that's not a good solution.

 

I'll look into using com.hp.hpl.jena.query.ResultSetFactory.copyResults so
we can clean-up the original ResultSet and be done with it.

 

Jennifer

 

 

From: Jennifer Cormier [mailto:jen...@at...] 
Sent: Thursday, August 26, 2010 9:30 AM
To: 'Graeme Stevenson'; 'ng4j-namedgraphs'
Subject: Re: [namedgraphs] Memory leak in database persistence
implementation

 

Good catch, Graeme!

 

Has anyone looked into this?

 

I think we'll want to have the find method return some sort of closeable
iterator, such as com.hp.hpl.jena.util.iterator.ClosableIterator, instead
of java.util.Iterator, and then the caller should call close if the
iterator is not fully traversed.  Similarly for method listGraphNames in
the same class (QuadDB).

 

I may try that and see .

 

Jennifer

 

 

From: Graeme Stevenson [mailto:ac...@gm...] 
Sent: Thursday, July 15, 2010 8:20 AM
To: ng4j-namedgraphs
Subject: [namedgraphs] Memory leak in database persistence implementation

 

Hi guys,

Firstly, thanks for all your hard work on the NG4J library. I've found it
extremely useful over the past couple of years. 

I've found a memory leak in the database persistence implementation in
version 0.9.3 caused by ResultSet objects not being closed when a
(positive) check is made for the existence of a Quad. The following
conditions reproduce the bug:

- A call is made to QuadDB.find(..) to check the existence of a Quad
- The hasNext() call on the returned iterator returns true
- No further methods are invoked on the iterator

The implementation of the QuadDB.find(..) currently only closes the
ResultSet object when hasNext() returns false, or an exception occurs.

There are two methods that call QuadDB.find(..) where this will always
occur when a match is found:
QuadDB.insert(...)
NamedGraphSetDB.containsQuads(...)

And this could potentially occur in the following methods if the iterator
is not completely traversed:
NamedGraphSetDB.findQuads(...)
NamedGraphDB.graphBaseFind(...)

Hope this is of use to you,

Cheers,
Graeme.

Re: [namedgraphs] Memory leak in database persistence implementation

[Jennifer C. <jen...@at...>]

What I've tried locally is to use a com.sun.rowset.CachedRowSet to capture
the contents of the ResultSet so that the ResultSet can be cleaned up.
This appears to be slightly reducing (i.e. hurting) performance in the
tests that I have run.  In theory I think it eliminates that particular
memory leak, but I don't have a good set of test data to confirm that.
(Anyone?)

 

I could provide the file to anyone who wants to try it (or attach it to a
bug report, or even commit it if it sounds reasonable).  A short
description of the change is as follows:

 

In QuadDB methods find and listGraphNames, 

.        final ResultSet results = executeQuery(sql); // change this to
not be final (not necessary but helps to find any errors later on)

.        Add this code following that line:
      // Use a CachedRowSet so we can clean-up the ResultSet

            // http://onjava.com/pub/a/onjava/2004/06/23/cachedrowset.html

            final CachedRowSetImpl crs;

            try {

                  crs = new CachedRowSetImpl();

                  crs.populate(results);

            } catch (SQLException e) {

                  throw new JenaException(e);

            } finally {

                  cleanUp(results);

         }

.        In the newly constructed Iterator for return, comment out all
calls to cleanup(results).  (The ResultSet is cleaned up in the finally
above, and there shouldn't be a need to clean-up the CachedRowSet.)  Also
can comment out the "if (!this.hasNext)" statement that only contains a
call to cleanUp.

.        In the newly constructed Iterator for return, replace all
occurrences of "results" with "crs".  (Each one should be showing an error
(if your IDE does that automatically like Eclipse) since results is no
longer final.)

 

One of my concerns is that this modification relies on a com.sun package.
I'd like to find a better solution .

 

Comments and corrections welcome!

 

Jennifer

 

 

From: Graeme Stevenson [mailto:ac...@gm...] 
Sent: Thursday, July 15, 2010 8:20 AM
To: ng4j-namedgraphs
Subject: [namedgraphs] Memory leak in database persistence implementation

 

Hi guys,

Firstly, thanks for all your hard work on the NG4J library. I've found it
extremely useful over the past couple of years. 

I've found a memory leak in the database persistence implementation in
version 0.9.3 caused by ResultSet objects not being closed when a
(positive) check is made for the existence of a Quad. The following
conditions reproduce the bug:

- A call is made to QuadDB.find(..) to check the existence of a Quad
- The hasNext() call on the returned iterator returns true
- No further methods are invoked on the iterator

The implementation of the QuadDB.find(..) currently only closes the
ResultSet object when hasNext() returns false, or an exception occurs.

There are two methods that call QuadDB.find(..) where this will always
occur when a match is found:
QuadDB.insert(...)
NamedGraphSetDB.containsQuads(...)

And this could potentially occur in the following methods if the iterator
is not completely traversed:
NamedGraphSetDB.findQuads(...)
NamedGraphDB.graphBaseFind(...)

Hope this is of use to you,

Cheers,
Graeme.