Menu
▾
▴
#21 SegmentsReader memory leak
SegmentsReader was not deleting its NormsCache so I
added the following :
void SegmentsReader::closeNorms() {
LOCK_MUTEX(norms_mutex);
map<const
char_t*,l_byte_t*,lucene::util::charCompare>::iterator
itr = normsCache.begin();
while (itr != normsCache.end
()) {
delete []itr-
>second;
itr ++;
}
UNLOCK_MUTEX(norms_mutex);
}
which I call in
void SegmentsReader::doClose() {
LOCK_MUTEX(doClose_LOCK);
for (int_t i = 0; i < readersLength;
i++){
readers[i]->close();
}
closeNorms();
UNLOCK_MUTEX(doClose_LOCK);
}
I also added
void closeNorms();
to segmentsReader.h as a private method.
It seems to work ok but I may not know entirely what I
am doing
Logged In: YES
user_id=671771
Actually the normsCache, which is an instance of VoidMap,
takes care of the deletion of its data upon destruction of the
object. This is specified in the call to the contructor in the
initializer list of SegmentsReader. Unfortunately the data in
the map can get corrupted, since it's being loaded with string
keys that may be deleted while still in use by the map. The
following code in the getNorms() method:
normsCache.put(const_cast<char_t*>(field), bytes);
should be changed to:
char_t* key = stringDuplicate(field);
normsCache.put(key, bytes);
Since we are now allocatiing the storage for the keys, we
need to tell the map to delete the keys when it is destroyed,
so, in the constructor's initializer list, change:
normsCache(false, util::DELETE_TYPE_NONE,
true, util::DELETE_TYPE_DELETE_ARRAY)
to:
normsCache(true, util::DELETE_TYPE_DELETE_ARRAY,
true, util::DELETE_TYPE_DELETE_ARRAY)
That should fix the problem.
Logged In: YES
user_id=414645
Are you (Jacquie) using version 0.8.12? On 2004.08.13
(after 0.8.12's release, I guess), I added the following
code to SegmentsReader's constructor:
-----------------------------------------------------------
// 2004.08.13: Fixed Erik-Kay-reported failure to free
normsCache's *values*.
normsCache.setDoDelete(false, DELETE_TYPE_DELETE_ARRAY);
// The memory underlying normsCache's *keys* is managed
by the Term class
// (char_t* Term::field), so normsCache mustn't free its
keys.
-----------------------------------------------------------
The "Erik-Kay-report" of which I spoke is here:
http://sourceforge.net/mailarchive/forum.php?thread_id=5293141&forum_id=33782
If I recall correctly, I reviewed the CLucene-internal code
that touches the char_t* field memory in order to verify
that a scenario such as the one Ken describes does not arise
within the CLucene itself. As long as the Term that manages
the char_t* field memory survives at least as long as the
SegmentsReader, corruption shouldn't occur. Even if my
memory of that code review and the correctness of its
conclusions can be trusted, I agree that it would be all too
easy for a client programmer to provoke such a scenario, so
unless Ben stops me, I'll implement Ken's suggestion.
Logged In: YES
user_id=1149323
Ken
You mean:
normsCache.setDoDelete(false, DELETE_TYPE_DELETE_ARRAY);
changing into:
normsCache.setDoDelete(true, DELETE_TYPE_DELETE_ARRAY);
right? Or are you talking about code that ain't in the
repository yet?
Logged In: YES
user_id=671771
John-
I think you are right. I added that line to the initializer list for
SegmentsReader. I guess you can get the same effect by
calling setDoDelete(), so either is probably ok.
Logged In: YES
user_id=1149323
Is there a reason this fix is not yet in the CVS repository?
Logged In: YES
user_id=414645
The reason is that my brief window of opportunity to work on
CLucene closed. I might be able to spend some time on it
this weekend, but I have both commercial clients and another
two open source projects making urgent demands on my time.
If you're sufficiently interested (the volume of your bug
reports suggests that you are), you might ask Ben for
developer rights to the CLucene SourceForge project so that
you can make CVS commits yourself. (I apologize to Ben if
I'm overstepping my bounds in making this suggestion; I am,
after all, just a contributor to this project, not a founder
or an administrator.)
Logged In: YES
user_id=1113468
I am sorry - I was looking at 0.8.11 because for some reason
I can't get 0.8.12 to link with the example/demo program. I
was testing things out with 0.8.12 and then seeing if the
problem showed up in the 0.8.11 demo and then comparing
the code. In this case I was getting a problem in 0.8.12 that
seemed to be related to this problem in 0.8.11 and forgot to
check the code. The actual problem turned out to be
something else entirely.
I have however implemented the changes suggested by Ken
and amended by John and all seems to be well.
Logged In: YES
user_id=1149323
Fixed in CVS
SegmentsReader.cpp 1.12
I implemented the fix posted by Ken and amend by me ;) and
commited the change in CVS.
Logged In: YES
user_id=1149323
normsCache.setDoDelete(true, DELETE_TYPE_DELETE_ARRAY); was
not good enough. The normcache did not delete its values,
which cause a memory leak. The suggestion by Ken, adding
normsCache(true, util::DELETE_TYPE_DELETE_ARRAY,
true, util::DELETE_TYPE_DELETE_ARRAY) is the proper solution
Logged In: YES
user_id=1149323
Fixed in CVS
SegmentsReader.cpp 1.13