On Wed, Sep 17, 2008 at 3:01 AM, Norval Hope <nrhope@gmail.com> wrote:
Hi All,

Just had a question about SearchHandler as I'm reviewing to see if
some memory leaks I experienced in the old <1.5.0 release code base
have been resolved. As best I can tell (apologies if I'm missing
something) there seem to be some different probs in the new
implementation regarding calling of
session.unregisterOutstandingRequest( req ) (the old impl had some
probs in the similar but now defunct use of SessionRegistry):
 1. Note that SearchHandler.handleIgnoringReferrals() always calls
session.registerOutstandingRequest( req ) immediately on entry

Yes this should happen.
 

 2. When it delegates to handleRootDseSearch() i don't see a
corresponding session.unregisterOutstandingRequest( req ) call, isn't
one required?

Yes you are right it should be required.  This is a bug which will cause a leak.
 

 3. I'm not sure what the right behaviour is for
handlePersistentSearch(), but I'd expect that logically
session.unregisterOutstandingRequest( req ) would need to be called
too (or in this case is the req considered to be outstanding
indefinitely?)

Right it need not be called in the case of psearch.  Here the only thing that will stop it is an abandon or the destruction of the connection/session.
 

 4. In the other normal search cases, shouldn't
session.unregisterOutstandingRequest( req ) be in a finally block so
it is also called when exceptions are encountered?

Yes indeed you are right again.  Another lousy leak.
 

 5. The old impl had a memory leak for searches when no results were
returned, as best I can tell the new cursor stuff doesn't suffer the
same problem but just wanted to throw this boundary case out there for
special consideration.

Best thing to do is write some test cases and see if we can produce a leak for this boundary case.  Should be pretty easily done.
 
FYI once we move to MINA 2.0 we can start looking at the slow client problem as well as hunting down more leaks in preparation for ADS 2.0.

Thanks,
Alex