Just to make sure we're on the same (code)page - I'm referring to this impl: https://github.com/apache/lucene.net/blob/trunk/src/core/Util/CloseableThreadLocal.cs
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.**** >>>> >>>> **** >>>> >>>> **** >>>> >>>> **** >>>> >>>> **** >>>> >>>> **** >>>> >>>> ** ** >>>> >>> >>> >> >
