RE: TestSearcherManager_Mem

[Shad Storhaug]

Parallel universe or not, I think you are making progress. I found a similar 
IncrementAndGet issue in the ThreadedIndexingAndSearchingTestCase that I have 
already corrected. However, it only mattered in one case, in all other cases 
the result of IncrementAndGet was not being utilized.

It is like someone intentionally wanted to make it impossible to get all of the 
bugs out of this code...

Anyway, stupid is as stupid does...I went through and scanned the entire 
codebase for IncrementAndGet and compared it against Lucene. Sure enough, the 
Core.Util.RefCount class was refactored from its original. I changed it back to 
the original code (backed by an AtomicInteger/AtomicInt32), and the 
TestCRTReopen() test no longer fails almost immediately - after a couple of 
minutes it now fails with the message "waited too long for commit generation". 
I don't know if I fixed it or broke it more, but it is definitely behaving 
differently now.

Now, let me see if I can bring your other changes into my universe...perhaps 
the new failure has something to do with the reset event.

Thanks,
Shad Storhaug (NightOwl888)


From: Van Den Berghe, Vincent [mailto:vincent.vandenber...@bvdinfo.com]
Sent: Thursday, March 23, 2017 7:04 PM
To: dev@lucenenet.apache.org
Cc: Shad Storhaug
Subject: TestSearcherManager_Mem

Even though I seem to live in a parallel universe where 42 isn't 42 and 4.8 
isn't 4.8, I'll have a stab at resolving TestSearcherManager_Mem.

First, there is a method in TrackingIndexWriter:

        public virtual long GetAndIncrementGeneration()
        {
            return indexingGen.IncrementAndGet();
        }


The implementation calls the wrong indexGen method: it should call 
GetAndIncrement(), which doesn't exist in the .NET version. You can add the 
method to the AtomicLong class.
Too bad there's no Interlocked.PostIncrement, but it's easy enough:

              public long GetAndIncrement()
              {
                     return Interlocked.Increment(ref value) - 1;
              }

And adjust the call accordingly:

        public virtual long GetAndIncrementGeneration()
        {
            return indexingGen.GetAndIncrement();
        }


Next we turn our attention to ControlledRealTimeReopenThread<T>.
There's an event defined as follows:

        private ManualResetEvent reopenCond = new ManualResetEvent(false);

This is not correct, since the remainder of the implementation only Sets and 
Waits, but never resets. Once the event Set, the wait will never ... uh... wait 
the second time around. Change this as follows:

        private AutoResetEvent reopenCond = new AutoResetEvent(false);

Next, for some reason, time is counted in nanoseconds, but since 
Environment.TickCount is in milliseconds, we need to convert it by multiplying 
by 1000000.
Unfortunately, this is done by multiplication:

Environment.TickCount * 1000000

Since Environment.TickCount is an int and 1000000 is an int, the result is 
negative unless you just rebooted your computer in a Tardis doing a polka 
backwards.
Define:

                private const long MS_IN_NS = 1000000;

... and change all other references to 1000000 except one (see below) with 
MS_IN_NS: this should solve the overflow problem using C#'s promotion rules.
Next, notice that 64-bit integers are sometimes read outside locks:

                searchingGen = refreshStartGen;
                if (targetGen > searchingGen)
                    while (targetGen > searchingGen)


This isn't guaranteed to be atomic, and I'm a curmudgeon when it comes to 
parallelism and atomicity. Change all these lines by:

                Interlocked.Exchange(ref searchingGen, refreshStartGen);
                if (targetGen > Interlocked.Read(ref searchingGen))
                    while (targetGen > Interlocked.Read(ref searchingGen))


In my own spacetime continuum, the test now passes.

Added bonus points: dispose of the waitable event in 
ControlledRealTimeReopenThread<T> Dispose method by adding:

                                  reopenCond.Dispose();

(after the Monitor.PulseAll(this); statement)

Extreme added bonus points: the following statement is incorrect, but it works 
anyway:

                  reopenCond.WaitOne(new TimeSpan(sleepNS / 1000000));//Convert 
NS to Ticks

(the 1000000 should not be replaced by MS_IN_NS in the new version) The reason 
why it's incorrect is because the argument to TimeSpan in the call accepts 
100-nanoseconds units, and dividing nanoseconds by 1000000 yields milliseconds 
instead. So you will wait a delay of a factor 10000 shorter. It turns out that 
the correction (divide by 100) will cause timeouts in the tests, so I left it 
as-is.


But all of this might be wrong, I may not even exists.


Vincent