> Does this include your changes to ClosableThreadLocal?
No, the old implementation.
DIGY
On Fri, Aug 26, 2011 at 1:45 PM, Ayende Rahien <aye...@ayende.com> wrote:
> Does this include your changes to ClosableThreadLocal?
>
> Also, you probably won't see a memory leak in this fashion. We only saw
> that in runs that exceeded 24 hours.
>
>
> On Thu, Aug 25, 2011 at 8:40 PM, Digy <digyd...@gmail.com> wrote:
>
>> ARGH! I should have done this long before.****
>>
>> When I run your test application (using Lucene.Net 2.9.4 in trunk) in a
>> loop, I see no memory leakage.****
>>
>> ** **
>>
>> private static void Main(string[] args)****
>>
>> {****
>>
>> for (int i = 0; i < 10; i++)****
>>
>> {****
>>
>> Main2(null);****
>>
>> Console.WriteLine(">>> MEM: " + GC.GetTotalMemory(true));
>> ****
>>
>> }****
>>
>> }****
>>
>> ** **
>>
>> DIGY.****
>>
>> ** **
>>
>> *From:* Ayende Rahien [mailto:aye...@ayende.com]
>> *Sent:* Thursday, August 25, 2011 10:23 AM
>> *To:* Itamar Syn-Hershko
>> *Cc:* Digy; lucene-net-user@lucene.apache.org
>>
>> *Subject:* Re: [Lucene.Net] Memory leaks****
>>
>> ** **
>>
>> Itamar,****
>>
>> You need to have some way of knowing that the item that you have there has
>> a Close method, no?****
>>
>> The only method that IDisposable have is Dispose(), IsDisposing and stuff
>> like that isn't part of it.****
>>
>> ** **
>>
>> The CTL need some way to close its values when it is done, but in order to
>> be able to do so, we need an interface to call it.****
>>
>> A lot of the stuff in Lucene implements Closable, which is basically
>> IDisposable****
>>
>> ** **
>>
>> On Thu, Aug 25, 2011 at 10:18 AM, Itamar Syn-Hershko <ita...@code972.com>
>> wrote:****
>>
>> Whats wrong in putting a call to Close() in Close() ?****
>>
>> ** **
>>
>> IDisposable in itself requires explicit request from the client (and can
>> be called on dtor), so it is no different from IDisposable in that regard.
>> And if you are calling Close() from client code, you might just as well do
>> the rest of the cleaning up there as well by calling subsequent Close()-es
>> from there.****
>>
>> ** **
>>
>> The only benefit of IDisposable is for lengthy disposing (the IsDisposing
>> or however thats called), and I'm pretty sure we can live without it here.
>> ****
>>
>> ** **
>>
>> On Thu, Aug 25, 2011 at 10:09 AM, Ayende Rahien <aye...@ayende.com>
>> wrote:****
>>
>> The code looks good.****
>>
>> What worries me is what happen when I have an object that has CTL that
>> reside inside the CTL.****
>>
>> That would mean that the second CTL would never be freed.****
>>
>> ** **
>>
>> Most specifically, look at FieldsReaderLocal, used in SegmentReader to
>> hold a FieldsReader.****
>>
>> The problem is that FieldsReader itself holds a CloseableThreadLocal
>> (fieldsStreamTL).****
>>
>> ** **
>>
>> Let us consider the case of a SegmentReader that is used in two threads,
>> then is closed.****
>>
>> Its thread local variables are closed, but nothing tells the FieldsReader
>> that _it_ is closed, so the fieldsStreamTL is never closed.****
>>
>> The valu*e *will only be cleaned when you the thread is closed, and that
>> may never happen (think thread pool threads, which are almost never allowed
>> to die).****
>>
>> ** **
>>
>> That is why I made the thing implement IDisposable, because you have
>> multiple levels of CTL.****
>>
>> ** **
>>
>> On Wed, Aug 24, 2011 at 11:05 PM, Digy <digyd...@gmail.com> wrote:****
>>
>> ****
>>
>> I created a new version of CloseableThreadLocal for 2.9.4g branch which
>> passes unit tests and is more similar to java version.****
>>
>> I implemented java's "ThreadLocal" class with two different approach. One
>> uses Thread.AllocateDataSlot (which is tooo slow) and the other one****
>>
>> uses the same technique in current implementation.****
>>
>> Propably It will not solve the memory-leakage problem, but It may be a
>> starting point to work on.****
>>
>> ****
>>
>> http://people.apache.org/~digy/CloseableThreadLocal.cs****
>>
>> http://people.apache.org/~digy/CloseableThreadLocal.java (v 3.0.2)****
>>
>> ****
>>
>> DIGY****
>>
>> ****
>>
>> *From:* Ayende Rahien [mailto:aye...@ayende.com]
>> *Sent:* Wednesday, August 24, 2011 4:26 PM
>> *To:* Itamar Syn-Hershko
>> *Cc:* lucene-net-user@lucene.apache.org; Digy****
>>
>>
>> *Subject:* Re: [Lucene.Net] Memory leaks****
>>
>> ****
>>
>> That make sense, and explains how it works.****
>>
>> Basically, there is no rooted hard reference anywhere to the values. It
>> looks like the actual problem is happening when you have instances in a
>> thread that is no longer used, the _values_ aren't weak references, and they
>> are leaking.****
>>
>> ****
>>
>> On Wed, Aug 24, 2011 at 3:41 PM, Itamar Syn-Hershko <ita...@code972.com>
>> wrote:****
>>
>> I moved this to a private conversation by accident, so back on the mailing
>> list now.****
>>
>> ****
>>
>> There seem to be a fundamental difference between the original Java impl
>> and Lucene.NET's, which may as well be causing the leak.****
>>
>> ****
>>
>> In the Java version ThreadLocal is a weak reference, and a hard reference
>> is persisted as well along with the thread holding it (using a synchronized
>> map). This allows to easily identify stale refs - if the thread holding the
>> weak ref is not alive anymore, the hard ref is removed and the GC can
>> process it.****
>>
>> ****
>>
>> In Lucene.NET's implementation only a static map of weak references is
>> kept. If I read this right, unlike Java's implementation where hard
>> references are kept and dereferenced from within the ThreadLocal when the
>> thread is dead, here there is nothing that will tell the GC to reclaim the
>> memory referenced by the Weak ref. The Weak-Hard combination that is missing
>> here is probably whats causing all this mess.****
>>
>> ****
>>
>> Ayende's solution which uses explicit disposal is virtually the missing
>> piece but in different clothing, since it requires changes to the core. If
>> the .NET version was made to match Java's that probably won't be necessary.
>> ****
>>
>> ****
>>
>> Before I rewrite CloseableThreadLocal to match the Java implementation -
>> is there a reason it was made this way?****
>>
>> ****
>>
>> On Wed, Aug 24, 2011 at 10:51 AM, Ayende Rahien <aye...@ayende.com>
>> wrote:****
>>
>> Itamar,****
>>
>> Can you try writing the same code in Java and see if we get the same
>> results?****
>>
>> ****
>>
>> On Tue, Aug 23, 2011 at 8:58 PM, Itamar Syn-Hershko <ita...@code972.com>
>> wrote:****
>>
>> How come the Java version doesn't suffer from this issue? if your answer
>> is going to be due to differences with the JVM, then IDisposable may well be
>> our best option here****
>>
>> ****
>>
>> On Tue, Aug 23, 2011 at 8:51 PM, Digy <digyd...@gmail.com> wrote:****
>>
>> Exactly, but I couldn’t find a nice solution yet. Any help welcomed J***
>> *
>>
>> ****
>>
>> Btw, Can you open a new issue for this ? ****
>>
>> Other people may also want to work on it.****
>>
>> ****
>>
>> https://issues.apache.org/jira/browse/LUCENENET****
>>
>> ****
>>
>> Thanks,****
>>
>> ****
>>
>> DIGY****
>>
>> ****
>>
>> *From:* Ayende Rahien [mailto:aye...@ayende.com]
>> *Sent:* Tuesday, August 23, 2011 8:32 PM****
>>
>>
>> *To:* Digy
>> *Cc:* Itamar Syn-Hershko
>> *Subject:* Re: [Lucene.Net] Memory leaks****
>>
>> ****
>>
>> I think that the main issue is that you have per thread state, but no way
>> of releasing this per thread state when you are closing the searcher.****
>>
>> On Tue, Aug 23, 2011 at 7:59 PM, Digy <digyd...@gmail.com> wrote:****
>>
>> Yes, IndexSearcher is expected to be thread safe. ****
>>
>> I am not saying, there is a misuse of it. Instead I try to understand the
>> real cause of it. IDisposable solution seems to hide the real bug.****
>>
>> ****
>>
>> DIGY****
>>
>> ****
>>
>> ****
>>
>> *From:* Ayende Rahien [mailto:aye...@ayende.com]
>> *Sent:* Tuesday, August 23, 2011 7:48 PM
>> *To:* Digy****
>>
>>
>> *Cc:* Itamar Syn-Hershko
>> *Subject:* Re: [Lucene.Net] Memory leaks****
>>
>> ****
>>
>> Digy,****
>>
>> IndexSearcher is thread safe, right?****
>>
>> From the docs (
>> http://lucene.apache.org/java/3_0_1/api/core/org/apache/lucene/search/IndexSearcher.html
>> ):****
>>
>> *NOTE*: IndexSearcher instances are completely thread safe, meaning
>> multiple threads can call any of its methods, concurrently. If your
>> application requires external synchronization, you should *not* synchronize
>> on the IndexSearcher instance; use your own (non-Lucene) objects instead.
>> ****
>>
>> The problem is that when you use an index searcher in a different thread
>> then the one you close it on, you are going to leak some memory. ****
>>
>> ****
>>
>> On Tue, Aug 23, 2011 at 7:35 PM, Digy <digyd...@gmail.com> wrote:****
>>
>> If I change the searching code as below, It works as expected****
>>
>> ****
>>
>> for (int i = 0; i < 150; i++)****
>>
>> {****
>>
>> ExecuteInParallel(() =>****
>>
>> {****
>>
>> var searchers = new[]****
>>
>> {****
>>
>> new IndexSearcher(directories[0], true),****
>>
>> new IndexSearcher(directories[1], true),****
>>
>> };****
>>
>> ****
>>
>> {****
>>
>> var topDocs = searchers[0].Search(new
>> TermQuery(new Term("val", "test")), 15);****
>>
>> for (int j = 0; j < topDocs.totalHits; j++)**
>> **
>>
>> {****
>>
>> searchers[0].Doc(j);****
>>
>> }****
>>
>> topDocs = searchers[1].Search(new
>> TermQuery(new Term("val", "test")), 15);****
>>
>> for (int j = 0; j < topDocs.totalHits; j++)**
>> **
>>
>> {****
>>
>> searchers[1].Doc(j);****
>>
>> }****
>>
>> TryAddSlot(slotsField,
>> allSlotsFromAllThreads);****
>>
>> };****
>>
>> ****
>>
>> ****
>>
>> foreach (var indexSearcher in searchers)****
>>
>> {****
>>
>> indexSearcher.Close();****
>>
>> }****
>>
>> });****
>>
>> }****
>>
>> ****
>>
>> ****
>>
>> Since IndexSearcher is closed in the thread where caching is done.****
>>
>> ****
>>
>> I have to think about it some more but I don’t think that adding
>> IDisposables to some classes would be a good solution. It is more related to
>> threading issues.****
>>
>> ****
>>
>> Anyway, I am working on it.****
>>
>> ****
>>
>> DIGY****
>>
>> ****
>>
>> ****
>>
>> *From:* Ayende Rahien [mailto:aye...@ayende.com]
>> *Sent:* Tuesday, August 23, 2011 6:05 PM
>> *To:* digy digy
>> *Cc:* Itamar Syn-Hershko
>> *Subject:* Re: [Lucene.Net] Memory leaks****
>>
>> ****
>>
>> It appears that we need to use searching as well to reproduce this.****
>>
>> Here is another version of the program.****
>>
>> I apologize for the code, but I basically needed to ensure that we would
>> get something resembling what we have in RavenDB, which is a multi threaded
>> access pattern.****
>>
>> ****
>>
>> On Tue, Aug 23, 2011 at 5:43 PM, digy digy <digyd...@gmail.com> wrote:***
>> *
>>
>> If I add ****
>>
>> GC.Collect(); ****
>>
>> after ****
>>
>> simpleAnalyzer.Close();****
>>
>> It outputs zeroes. Am I missing something?****
>>
>> ****
>>
>> DIGY****
>>
>> ****
>>
>> On Tue, Aug 23, 2011 at 5:07 PM, Itamar Syn-Hershko <ita...@code972.com>
>> wrote:****
>>
>> See attached.****
>>
>> ****
>>
>> The key to this problem is multi-threading. In this sample app, some of
>> the threads do not dispose of their resources properly, even though Close is
>> called correctly.****
>>
>> ****
>>
>> The suggested patch is confirmed to fix this behavior, but there may be
>> other pit-falls our use cases didn't reveal.****
>>
>> ****
>>
>> Itamar.****
>>
>> ****
>>
>> On Tue, Aug 23, 2011 at 4:19 PM, digy digy <digyd...@gmail.com> wrote:***
>> *
>>
>> Hi Itamar,****
>>
>> ****
>>
>> Can you send a simple self-contained test case showing the bug?****
>>
>> ****
>>
>> DIGY****
>>
>> ****
>>
>> On Tue, Aug 23, 2011 at 2:41 PM, Itamar Syn-Hershko <ita...@code972.com>
>> wrote:****
>>
>> Hi,****
>>
>> ****
>>
>> We recently discovered another memory leak within Lucene.NET, that is
>> related to LUCENENET-358 (CloseableThreadLocal memory leak in
>> LocalDataStoreSlot). As it turns out, disposables weren't disposed properly.
>> With RavenDB, that caused memory consumption to always grow.****
>>
>> ****
>>
>> We ended up modifying some core files to assure proper disposal, and
>> things are looking better now. We are still testing, and will follow up if
>> more fixes will be required.****
>>
>> ****
>>
>> Here are the relevant diffs in our fork of Lucene.NET:****
>>
>> ****
>>
>>
>> https://github.com/ayende/ravendb/commit/a5031049599968f85e733f22a1ed568c09703c30
>> ****
>>
>>
>> https://github.com/ayende/ravendb/commit/bd9ca1ab154732d27985c2d237917af8bbd5fbf3
>> ****
>>
>> ****
>>
>> The full discussion and bug hunting is here:****
>>
>> ****
>>
>>
>> http://groups.google.com/group/ravendb/browse_thread/thread/9c6340987407aece/1d86d6a61175233c
>> ****
>>
>> ****
>>
>> I'm also attaching a diff between our fork to the Lucene.Net_2_9_2_RC2
>> tag. Will be happy to see this incorporated into 2.9.4 (and for it to be
>> released already...).****
>>
>> ****
>>
>> Itamar.****
>>
>> ****
>>
>> ****
>>
>> ****
>>
>> ****
>>
>> ****
>>
>> ****
>>
>> ****
>>
>> ****
>>
>> ****
>>
>> ****
>>
>> ** **
>>
>> ** **
>>
>> ** **
>>
>
>
