Fix IndexWriter.Dispose breaking the standard Dispose contract, #1399#1426
Draft
paulirwin wants to merge 20 commits into
Draft
Fix IndexWriter.Dispose breaking the standard Dispose contract, #1399#1426paulirwin wants to merge 20 commits into
paulirwin wants to merge 20 commits into
Conversation
apache#1284 Backport of LUCENE-5871 (upstream commit 2cfcdcc on branch_4x). If anything before the cleanup steps in the close path throws — for example CommitInternal due to an I/O failure — the old straight-line CloseInternal skipped DropAll, deleter.Dispose(), and writeLock.Dispose(), leaking the reader pool, deleter, and write.lock file handle. Replaces CloseInternal with a Shutdown(bool) method that runs Flush, FinishMerges, CommitInternal, then RollbackInternal in a try/finally so RollbackInternal always runs and releases all resources. Also moves the numDocsInRAM accounting in DocumentsWriter into a finally block so a partially-applied addDocument is still counted correctly. Tests added: - TestHasUncommittedChangesAfterException, TestDoubleClose, TestRollbackThenClose, TestCloseThenRollback, TestRollbackWhileMergeIsRunning, TestCloseDuringCommit (TestIndexWriter) - TestOutOfMemoryErrorRollback (TestIndexWriterExceptions) - TestDisposeDoesNotLeakWriteLockOnCommitFailure ([LuceneNetSpecific] regression test that explicitly verifies write.lock is released after a CommitInternal IOException) TestOtherFiles2 was removed in the upstream commit — its assertion no longer holds because a graceful close now checkpoints unreferenced files. Kept commented out at its original location for porting reference. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Follow-up to 7ec8790. Audit against upstream 2cfcdcc found four hunks from the same commit that weren't ported and three doc/annotation changes on the close() overloads. This commit closes all seven gaps. Test/test-framework hunks (from 2cfcdcc): - TestIndexWriterCommit.TestPrepareCommitThenClose: new test, pins the Shutdown contract that close() with an outstanding prepareCommit throws IllegalStateException. - RandomIndexWriter.Dispose: add `IndexWriter.IsClosed == false` guard around the 1-in-8 random force-merge. Without it the new Shutdown contract can leave the writer closed when the test's user-visible call threw, and the force-merge in teardown then throws AlreadyClosedException. - TestIndexWriterExceptions: w.Commit() before w.Dispose() in the random- close branch so Shutdown doesn't throw IllegalStateException there. New test file (from LUCENE-5644 at the 4.8.1 release point, with the 2cfcdcc tweaks layered on TestManyThreadsClose): - TestIndexWriterThreadsToSegments: four tests covering segment-flush- per-thread invariants. Uses the 4.8.1-era APIs (NoMergePolicy.NO_COMPOUND_FILES, Lucene46 codec, 4-arg SegmentCommitInfo). IndexWriter doc + annotation changes (from 2cfcdcc): - Dispose(): replace the pre-fix doc text (which described the old "write lock may remain held / call close() again" recovery flow) with the new two-cases bullet list. - Dispose(bool): replace the OOME warning with a cross-reference to Dispose() for exception behavior; expand the merge-starvation note. - Dispose(bool): mark [Obsolete] mirroring upstream's @deprecated. The message and intent are identical: prefer Commit() + Rollback() to abort merges and then close. LUCENE-5871 and LUCENE-5644 both first appear in 4.10.0 (post-4.8.1), but are in scope because the project tracks branch_4x at 4.8.1. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…pache#1284 Follow-up to f404981. Renames the four extracted Java anonymous Thread classes in TestIndexWriterThreadsToSegments to match the Lucene.NET convention of <BaseType>AnonymousClass with a method-specific suffix when needed: - SegmentCountOnFlushBasicThread -> ThreadAnonymousClassForSegmentCountOnFlushBasic - SegmentCountOnFlushRandomThread -> ThreadAnonymousClassForSegmentCountOnFlushRandom - ManyThreadsCloseThread -> ThreadAnonymousClassForManyThreadsClose - DocsStuckInRAMForeverThread -> ThreadAnonymousClassForDocsStuckInRAMForever CheckSegmentCount is left as-is because it is a named static class in upstream Java, not an anonymous inner class. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…vals, apache#1284 Upstream 2cfcdcc rewrote close(boolean) to delegate to shutdown(boolean) but kept assertEventQueueAfterClose() and closeInternal(boolean, boolean) as private dead code. The original apache#1284 port (7ec8790) dropped both, which matches trunk LUCENE-4246's eventual cleanup (8559eaf, Lucene 5.0) but diverges from the conservative branch_4x state at 2cfcdcc. Adds two LUCENENET breadcrumbs around ShouldClose() so a future porter diffing against 2cfcdcc can find the removal rationale at the position where each method used to live: - Full note before ShouldClose() covering both, since assertEventQueueAfterClose() sat there in upstream. - One-liner after ShouldClose() pointing back to the full note, since closeInternal() sat immediately after. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
apache#1284 Follow-up to 08d82e8. The previous rename pass only covered the four classes in TestIndexWriterThreadsToSegments and missed three classes that the original apache#1284 port extracted from anonymous inner classes in TestIndexWriter: - RollbackWhileMergeInfoStream -> InfoStreamAnonymousClassForRollbackWhileMergeIsRunning - RollbackWhileMergeScheduler -> ConcurrentMergeSchedulerAnonymousClassForRollbackWhileMergeIsRunning - SlowCommittingInfoStream -> InfoStreamAnonymousClassForCloseDuringCommit The ForCloseDuringCommit suffix matches the existing ThreadAnonymousClassForCloseDuringCommit precedent. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a Lucene.Net.Support.Threading.CountDownLatch class as a 1:1 port of java.util.concurrent.CountDownLatch (adapted from Apache Harmony), along with a JSR166-style CountDownLatchTest port. Why this exists alongside System.Threading.CountdownEvent: - CountdownEvent.Signal() throws InvalidOperationException if called after the count reaches zero. Java's CountDownLatch.countDown() is idempotent at zero. Test code ported from upstream Lucene Java routinely calls countDown() past zero (e.g. one thread per loop iteration when only the first N matter); the C# port has to either wrap each call in a CurrentCount > 0 guard or eat sporadic exceptions during teardown. - This bug was hit by TestConcurrentMergeScheduler.TestMaxMergeCount on CI after the LUCENE-5871 / apache#1284 port made Shutdown(false) forward-flush merges more aggressively, pushing more merges through DoMerge than maxMergeCount and tripping the Signal() overshoot. The new CountDownLatch wraps a ManualResetEventSlim + Interlocked int. CountDown is safe past zero (early return + idempotent Set). Count clamps at zero to match Java's getCount() semantics. Await(TimeSpan) returns a bool matching await(long, TimeUnit). Thread interrupt is NOT honored (consistent with the rest of Lucene.NET's threading; see the ReentrantLockTest precedent), and the two interrupt-dependent tests in CountDownLatchTest are [Ignore]'d with a LUCENENET note. CountDownLatchTest extends the existing JSR166TestCase. Anonymous Java inner classes are extracted as ThreadAnonymousClassFor<TestName> per the established naming convention. 7 of 9 upstream tests pass; 2 are skipped per the interrupt-semantics note above. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…e#1284 Replaces System.Threading.CountdownEvent with the new Lucene.Net.Support.Threading.CountDownLatch across 22 test files where the upstream Java equivalent uses java.util.concurrent.CountDownLatch. Mechanical changes: - type rename: CountdownEvent -> CountDownLatch - .Signal() -> .CountDown() - .Wait() / .Wait(TimeSpan) -> .Await() / .Await(TimeSpan) - .CurrentCount -> .Count - .IsSet -> .Count == 0 - added using Lucene.Net.Support.Threading where needed Workarounds for CountdownEvent.Signal()'s "throws past zero" behavior that are now redundant (CountDown is idempotent at zero, matching Java): - TestConcurrentMergeScheduler.TestMaxMergeCount: removed the CurrentCount > 0 guard. This was the trigger for the CI failure on Windows .NET Framework and Ubuntu .NET 8 after the LUCENE-5871 port made Shutdown(false) flush more aggressively. - TestIndexWriterWithThreads: removed the same pre-existing guard at iwConstructed. - TestDocumentsWriterStallControl: removed if (!updateJoin.IsSet) guard around CountDown(); upstream calls countDown() unconditionally. - TestControlledRealTimeReopenThread: replaced two signal.Reset(Count - 1)/latch.Reset(Count - 1) hacks with plain CountDown(), matching upstream signal.countDown()/latch.countDown(). Other adjustments: - BaseDocValuesFormatTestCase: dropped `using` keyword on two latches. CountdownEvent was IDisposable; CountDownLatch isn't (Java's isn't either), so the latches now go out of scope without explicit cleanup. - TestControlledRealTimeReopenThread.LatchedIndexWriter: ctor changed from public to internal so CountDownLatch (internal) is reachable from a public class member. Files touched: 22 test files plus 2 in TestFramework. All ported sites now read the same as the upstream Java; no LUCENENET notes were added because the code is now equivalent. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…che#1284 Align the latch's naming with .NET's CountdownEvent: rename the type CountDownLatch -> CountdownLatch, Await()/Await(TimeSpan) -> Wait()/Wait(TimeSpan), and CountDown() -> Signal(). Upstream-Java references in comments are left as-is. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ilarity selection MockRandomPostingsFormat, RandomCodec, and RandomSimilarityProvider chose a per-file/per-field int-stream factory, postings/docvalues format, and similarity by indexing into a collection with `string.GetHashCode()`. On .NET, string hash codes are randomized per process, so a choice made when writing an index (e.g. in the forked child process of TestIndexWriterOnJRECrash) differed from the choice made when reading it back in another process, producing spurious "codec header mismatch" / CheckIndex failures. Java is unaffected because String.hashCode() is deterministic across JVMs. Use J2N's CharSequenceComparer.Ordinal.GetHashCode(), which is deterministic and produces the same value as Java's String.hashCode(), keeping writer and reader in sync. This was a pre-existing test-framework bug (reproduces on master). Verified: TestIndexWriterOnJRECrash.TestNRTThreads_Mem (previously a deterministic failure under seed 0x03c0703dd1d74190:0xfc8ded704ed908f9) now passes on fixed and random seeds; RandomCodec- and similarity-heavy suites remain green. Confirmed against a native build of upstream Lucene 4.8.1, which passes this test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
paulirwin
added
the
notes:breaking-change
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix IndexWriter.Dispose breaking the standard Dispose contract
Fixes #1399
Description
DO NOT MERGE: This is a stacked PR on #1311, which is why it is a draft. #1311 must be merged first, then this PR should be rebased to remove the #1311 commits. If you want to review this draft, please see only commit fadeb23 (or any added after it, if there is feedback/changes).
This resolves the tension of IndexWriter.Dispose breaking the standard Dispose contract, by having an extra boolean parameter of
waitForMerges. See the original motivation in #1399.After evaluating a few different paths, I chose to go with a
public Close(bool waitForMerges)method that is marked deprecated. This seems weird, to introduce a new public method while at the same time making it deprecated, and deviating from our Close/Dispose typical mapping. But here is my justification:using) anyways, which already callsShutdown(true).Closein specific situations where it is warranted in place ofDispose, like inTokenStream.I also added a virtual
void Dispose(bool disposing)method so that users with custom implementations that inherit from IndexWriter can override this and clean up their resources. This is called afterShutdowninDispose().All tests were updated to call
Close(bool)instead ofDispose(bool). Because this method is obsolete, the#pragmasuppressions remain in place.This is a breaking change because of the removal of the
Dispose(bool, bool)overload and makingDispose(bool)go from public to protected.