Memory Leak in Persistent search ?

• Key: DIRSERVER-644
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Blocker
• Assignee: Emmanuel Lecharny
• Reporter: Emmanuel Lecharny
• Component/s: core
• Affects Version/s: 1.5.4, 1.0-RC3
• Fix Version/s: 1.5.5

Description

After having profiled memory, it seems we have a memory leak in SessionRegistry.

A little test (attached) does a search N times for N threads, and for each search, a OutstandingRequest is attached to the session. After a few thousands of search we fall in OOM. I've put some trace in those methods :
SessionRegistry.addOutstandingRequest
and
SessionRegistry.removeOutstandingRequest

Session Released
addOutstandingRequest 2
addOutstandingRequest 3
addOutstandingRequest 4
... ( 100 requests)
addOutstandingRequest 99
addOutstandingRequest 100
addOutstandingRequest 101
remove session

The SessionRegistry.removeOutstandingRequest is never called, except if an exception is raised (NamingException).

It may be on purpose ( persistent search), but we can't assume the server will be able to hold as many OutstandingRequest as we have search requests - or entries -.

Comments

[Emmanuel Lecharny]

The test used.

[Alex Karasulu]

This actually has nothing to do with psearch but obviously psearch benefits the most from it since they never terminate ;). Basically we store outstanding long running requests in the session and remove all this stuff when MINA tell's us the connection has dropped.

Perhaps the mechanism we have setup is not working correctly and the requests are collecting. We need to get Trustin's input on this.

[Emmanuel Lecharny]

That's good news !

However, requests are not supposed to be long here : they are simple search with a DN as key. The request must be discarded when the result has been sent to the client, not when the session dies.

Now, is it necesary to store this request ? What is this good for? If we have a very long request, like a search through all the base, then we will have three cases :
1) there is a time limit : we returns the data we found at this point.
2) there is a size limit : we returns the data we found at this point.
3) there are no limit : we wait for the server to build the full result, and we send it back to the client.

I any case, we don't need the Request to be stored, am I totally wrong ?

[Alex Karasulu]

Man I was totally wrong about this. We do not remove outstanding requests at the end of a search. What we need to do is make the SearchResponseIterator remove the outstanding request from the session when it sends a response done result.

[Alex Karasulu]

Fix committed revision 414011:


http://svn.apache.org/viewvc?view=rev&revision=414011

[Emmanuel Lecharny]

Closed after the fix has been applied

[Emmanuel Lecharny]

Reopen the issue as the patch seems to have been applied in a branch but not have been merged in trunk.

[Simon Temple]

I think the original patch has made it through although it's now in different classes due to refactoring.

However, I think there is a leak if a search is valid but unsuccessful. I.e. If the search returns an empty enumeration the server side registry continues to hold a reference to the search session.

Here's an example client snippet I used to re-create the problem:

<snip>
        Attributes matchAttrs = new BasicAttributes( "objectClass", "typeinfo", true );
        NamingEnumeration<SearchResult> en = null;
        en = ctx.search( name, matchAttrs, new String[] { "networkid" } );
        en.close( );
<snip>

My proposed fix to the 1.5.2 code is in DefaultSearchHandler, in searchMessageReceived() Line# 373:

            if ( list.hasMore() )
            {
                <snip>
            }
            else
            {
                <snip>

                // SPT - empty result lists should not accumulate registered sessions
                getSessionRegistry().removeOutstandingRequest( session, req.getMessageId() );
            }

I see there is a persistent search implementation that may also suffer the same problem.

Comments appreciated.

[Emmanuel Lecharny]

The initial issue has been fixed when we moved to cursor implementation.

[Emmanuel Lecharny]

Fixed a while ago.