389741 - ldap_memcache_flush() doesn't clear negative results

Status: RESOLVED FIXED

(Directory :: LDAP C SDK, defect)

Description

[Ulf Weltman]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.5) Gecko/20070713 Firefox/2.0.0.5
Build Identifier: 

ldap_memcache_flush() seems to only clear out cache items that contain messages of search entry type with a DN that matches the base that was based to ldap_memcache_flush().  But searches that didn't find an entry would only cache a search result message and no search entries.

In memcache.c:memcache_access() when "mode == MEMCACHE_ACCESS_FLUSH" it's skipping the cache slot if (!NSLDAPI_IS_SEARCH_ENTRY( pMsg->lm_msgtype )).  I think we should flush it if there are no search entries, and the base in the search results header matches the base passed in to ldap_memcache_flush().

Reproducible: Always

Comment 1

[Mark Smith [:mcs]]

The memcache code is not widely used and not well tested (it was written by a messaging server engineer a long time ago, but most people are better off implementing their own cache inside their application).  Do you know of any mainstream applications that use it?

In any case, this does sound like a bug.  Since there is not much documentation in the code, it is hard to tell what the original author's intent was though.

Comment 2

[Ulf Weltman]

Not mainstream but there's a proprietary application (not from me) used by many institutions around the world.  I think I will need to fix it, maybe I could add an additional memcache_access() mode like MEMCACHE_ACCESS_FLUSH_EXT so I don't break the existing MEMCACHE_ACCESSS_FLUSH behavior in case someone relies on it?

Comment 3

[Mark Smith [:mcs]]

(In reply to comment #2)
> Not mainstream but there's a proprietary application (not from me) used by many
> institutions around the world.

Is the application open source?  Where would one get a copy?

> ... I think I will need to fix it, maybe I could
> add an additional memcache_access() mode like MEMCACHE_ACCESS_FLUSH_EXT so I
> don't break the existing MEMCACHE_ACCESSS_FLUSH behavior in case someone
> relies on it?

Good idea.  Maybe use the name MEMCACHE_ACCESS_FLUSH_RESULTS (or something else that is more descriptive than ..._EXT).

Comment 4

[Ulf Weltman]

Attachment: patch proposal

The application is closed source unfortunately, I haven't seen it either.

This patch adds a new LDAP API call, ldap_memcache_flush_results() which works the same as ldap_memcache_flush() except it'll also clear out results from search requests that didn't return any entries.

Comment 5

[Mark Smith [:mcs]]

The patch looks good to me.  Do you have any tests for the memcache code that could be added to the source tree?  This really seems like a bug or odd feature that we should have known about long ago....

Comment 6

[Ulf Weltman]

Can you add a + for the review if it's OK?  Also, Rich and Nathan, any concerns about the changes?

Regarding tests, I was using a modified copy of the csearch example but I don't think the examples programs are a good place for tests.  What did you have in mind?

Comment 7

[Mark Smith [:mcs]]

Comment on attachment 275868 [details] [diff] [review]
patch proposal

Forgot to mark patch as OK (oops).

Comment 8

[Mark Smith [:mcs]]

(In reply to comment #6)
> Regarding tests, I was using a modified copy of the csearch example but I don't
> think the examples programs are a good place for tests.  What did you have in
> mind?

It would be nice to start creating tests that we can commit to CVS and run. But we have never gotten a lot of traction in that area for the LDAP C SDK.  If the changes to csearch are not too ugly, attach them to this bug so we at least have a way to revisit this bug if we need to do so.

Comment 9

[Ulf Weltman]

Checking in directory/c-sdk/ldap/libraries/libldap/memcache.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/memcache.c,v  <--  memcache.c
new revision: 5.7; previous revision: 5.6
done
Checking in directory/c-sdk/ldap/include/ldap-extension.h;
/cvsroot/mozilla/directory/c-sdk/ldap/include/ldap-extension.h,v  <--  ldap-extension.h
new revision: 5.9; previous revision: 5.8
done