ldap_memcache_flush() doesn't clear negative results

RESOLVED FIXED

Status

RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: ulf, Assigned: ulf)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

11 years ago
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

11 years ago
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.
(Assignee)

Comment 2

11 years ago
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

11 years ago
(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).
(Assignee)

Comment 4

11 years ago
Created attachment 275868 [details] [diff] [review]
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.
(Assignee)

Updated

11 years ago
Attachment #275868 - Flags: review?(mcs)

Comment 5

11 years ago
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....
Assignee: mcs → ulf
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 6

11 years ago
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

11 years ago
Comment on attachment 275868 [details] [diff] [review]
patch proposal

Forgot to mark patch as OK (oops).
Attachment #275868 - Flags: review?(mcs) → review+

Comment 8

11 years ago
(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.

(Assignee)

Comment 9

10 years ago
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
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.