Skip to content

Commit

Permalink
BREAKING: Lucene.Net.Index.IndexWriter: Fixed Dispose() overloads so …
Browse files Browse the repository at this point in the history
…there is no method signature conflict between the public Dispose(waitForMerges) method and the protected Dispose(disposing) method. See apache#265.
  • Loading branch information
NightOwl888 committed Nov 7, 2022
1 parent dfae964 commit a26bd2b
Show file tree
Hide file tree
Showing 3 changed files with 108 additions and 12 deletions.
2 changes: 1 addition & 1 deletion src/Lucene.Net.TestFramework/Index/RandomIndexWriter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -443,7 +443,7 @@ public void Dispose()

/// <summary>
/// Dispose this writer. </summary>
/// <seealso cref="IndexWriter.Dispose(bool)"/>
/// <seealso cref="IndexWriter.Dispose()"/>
protected virtual void Dispose(bool disposing)
{
if (disposing)
Expand Down
116 changes: 106 additions & 10 deletions src/Lucene.Net/Index/IndexWriter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.ComponentModel;
using System.Diagnostics.CodeAnalysis;
using System.Globalization;
using System.IO;
using System.Runtime.CompilerServices;
Expand Down Expand Up @@ -1043,7 +1045,7 @@ private void MessageState()
/// This is a "slow graceful shutdown" which may take a long time
/// especially if a big merge is pending: If you only want to close
/// resources use <see cref="Rollback()"/>. If you only want to commit
/// pending changes and close resources see <see cref="Dispose(bool)"/>.
/// pending changes and close resources see <see cref="IndexWriterExtensions.Dispose(IndexWriter, bool)"/>.
/// <para/>
/// Note that this may be a costly
/// operation, so, try to re-use a single writer instead of
Expand Down Expand Up @@ -1095,29 +1097,81 @@ public void Dispose()
}

/// <summary>
/// Closes the index with or without waiting for currently
/// Disposes the index while waiting for currently
/// running merges to finish and optionally releases the managed resources.
///
/// <para><b>NOTE</b>: If this method hits an <see cref="OutOfMemoryException"/>
/// you should immediately dispose the writer, again. See
/// <see cref="IndexWriter"/> for details.</para>
///
/// <para><b>NOTE</b>: Always override <see cref="Dispose(bool, bool)"/> instead of this overload.</para>
///
/// <para><b>NOTE</b>: The public extension method <see cref="IndexWriterExtensions.Dispose(IndexWriter, bool)"/>
/// has the same method signature as this method. However, that method is intended for external calls by Lucene users,
/// where this one is intended to be used by subclasses to complete the
/// <a href="https://learn.microsoft.com/en-us/dotnet/standard/garbage-collection/implementing-dispose#the-disposebool-method-overload">
/// .NET dispose pattern</a>.</para>
/// </summary>
/// <param name="disposing"><c>true</c> to release both managed and unmanaged resources;
/// <c>false</c> to release only unmanaged resources. </param>
// LUCENENET specific - This overload only exists to implement the dispose pattern, and should not be overridden because
// it doesn't necessarily capture all of the Dispose() calls like Dispose(bool disposing, bool waitForMerges) does.
// This only exists to keep code analyzers from complaining that we are out of spec. Finalizers and should call Dispose(true, true).
[MethodImpl(MethodImplOptions.NoInlining)]
[EditorBrowsable(EditorBrowsableState.Never)]
protected virtual void Dispose(bool disposing)
{
Dispose(disposing, waitForMerges: true);
}

/// <summary>
/// Disposes the index with or without waiting for currently
/// running merges to finish. This is only meaningful when
/// using a <see cref="MergeScheduler"/> that runs merges in background
/// threads.
///
/// <para><b>NOTE</b>: if this method hits an <see cref="OutOfMemoryException"/>
/// <para>This call will block
/// until all merges complete; else, it will ask all
/// running merges to abort, wait until those merges have
/// finished (which should be at most a few seconds), and
/// then return.
/// </para>
///
/// <para><b>NOTE</b>: When overriding, this overload should always be used instead of <see cref="Dispose(bool)"/>
/// to ensure calls from <see cref="Dispose()"/> and <see cref="IndexWriterExtensions.Dispose(IndexWriter, bool)"/>
/// are both handled. Always be sure to call <c>base.Dispose(disposing, waitForMerges)</c> when overriding this method.</para>
///
/// <para><b>NOTE</b>: If this method hits an <see cref="OutOfMemoryException"/>
/// you should immediately dispose the writer, again. See
/// <see cref="IndexWriter"/> for details.</para>
///
/// <para><b>NOTE</b>: it is dangerous to always call
/// <c>Dispose(false)</c>, especially when <see cref="IndexWriter"/> is not open
/// <para><b>NOTE</b>: It is dangerous to always call
/// with <paramref name="waitForMerges"/> set to <c>false</c>,
/// especially when <see cref="IndexWriter"/> is not open
/// for very long, because this can result in "merge
/// starvation" whereby long merges will never have a
/// chance to finish. This will cause too many segments in
/// your index over time.</para>
/// </summary>
/// <param name="waitForMerges"> if <c>true</c>, this call will block
/// <param name="waitForMerges"> If <c>true</c>, this call will block
/// until all merges complete; else, it will ask all
/// running merges to abort, wait until those merges have
/// finished (which should be at most a few seconds), and
/// then return. </param>
/// <param name="disposing"><c>true</c> to release both managed and unmanaged resources;
/// <c>false</c> to release only unmanaged resources. </param>
// LUCENENET specific - Added this overload to allow subclasses to dispose resoruces
// in one place without also having to override Dispose(bool).
[MethodImpl(MethodImplOptions.NoInlining)]
public virtual void Dispose(bool waitForMerges) // LUCENENET TODO: API - mark protected
protected internal virtual void Dispose(bool disposing, bool waitForMerges)
{
if (disposing)
{
Close(waitForMerges);
}
}

private void Close(bool waitForMerges)
{
// Ensure that only one thread actually gets to do the
// closing, and make sure no commit is also in progress:
Expand Down Expand Up @@ -2196,7 +2250,7 @@ internal string NewSegmentName()
/// you should immediately dispose the writer. See
/// <see cref="IndexWriter"/> for details.</para>
///
/// <para><b>NOTE</b>: if you call <see cref="Dispose(bool)"/>
/// <para><b>NOTE</b>: if you call <see cref="IndexWriterExtensions.Dispose(IndexWriter, bool)"/>
/// with <c>false</c>, which aborts all running merges,
/// then any thread still running this method might hit a
/// <see cref="MergePolicy.MergeAbortedException"/>.</para>
Expand Down Expand Up @@ -2378,7 +2432,7 @@ private bool MaxNumSegmentsMergesPending()
/// you should immediately dispose the writer. See
/// <see cref="IndexWriter"/> for details.</para>
///
/// <para><b>NOTE</b>: if you call <see cref="Dispose(bool)"/>
/// <para><b>NOTE</b>: if you call <see cref="IndexWriterExtensions.Dispose(IndexWriter, bool)"/>
/// with <c>false</c>, which aborts all running merges,
/// then any thread still running this method might hit a
/// <see cref="MergePolicy.MergeAbortedException"/>.</para>
Expand Down Expand Up @@ -3434,7 +3488,7 @@ public virtual void AddIndexes(params Directory[] dirs)
/// <see cref="DirectoryReader.Open(Directory, int)"/>.
///
/// <para/>
/// <b>NOTE</b>: if you call <see cref="Dispose(bool)"/> with <c>false</c>, which
/// <b>NOTE</b>: if you call <see cref="IndexWriterExtensions.Dispose(IndexWriter, bool)"/> with <c>false</c>, which
/// aborts all running merges, then any thread still running this method might
/// hit a <see cref="MergePolicy.MergeAbortedException"/>.
/// </summary>
Expand Down Expand Up @@ -6412,4 +6466,46 @@ private static bool SlowFileExists(Directory dir, string fileName)
}
}
}

/// <summary>
/// Extensions to <see cref="IndexWriter"/>.
/// </summary>
public static class IndexWriterExtensions
{
/// <summary>
/// Disposes the index with or without waiting for currently
/// running merges to finish. This is only meaningful when
/// using a <see cref="MergeScheduler"/> that runs merges in background
/// threads.
///
/// <para><b>NOTE</b>: If this method hits an <see cref="OutOfMemoryException"/>
/// you should immediately dispose the writer, again. See
/// <see cref="IndexWriter"/> for details.</para>
///
/// <para><b>NOTE</b>: It is dangerous to always call
/// <c>Dispose(false)</c>, especially when <see cref="IndexWriter"/> is not open
/// for very long, because this can result in "merge
/// starvation" whereby long merges will never have a
/// chance to finish. This will cause too many segments in
/// your index over time.</para>
/// </summary>
/// <param name="waitForMerges"> If <c>true</c>, this call will block
/// until all merges complete; else, it will ask all
/// running merges to abort, wait until those merges have
/// finished (which should be at most a few seconds), and
/// then return. </param>
/// <exception cref="ArgumentNullException"><paramref name="indexWriter"/> is <c>null</c>.</exception>
// LUCENENET specific - added to allow a public call to Dispose(bool) (where the parameter means
// waitForMerges) for external use, while still using a protected Dispose(bool) (where the parameter
// means being called from Dispose() vs finalizer) when implementing subclasses of IndexWriter.
[SuppressMessage("Usage", "CA1816:Dispose methods should call SuppressFinalize", Justification = "This is Lucene's alternate path to Dispose() and we must suppress the finalizer here.")]
public static void Dispose(this IndexWriter indexWriter, bool waitForMerges)
{
if (indexWriter is null)
throw new ArgumentNullException(nameof(indexWriter));

indexWriter.Dispose(disposing: true, waitForMerges);
GC.SuppressFinalize(indexWriter);
}
}
}
2 changes: 1 addition & 1 deletion src/Lucene.Net/Index/MergePolicy.cs
Original file line number Diff line number Diff line change
Expand Up @@ -540,7 +540,7 @@ protected MergeException(SerializationInfo info, StreamingContext context)

/// <summary>
/// Thrown when a merge was explicity aborted because
/// <see cref="IndexWriter.Dispose(bool)"/> was called with
/// <see cref="IndexWriterExtensions.Dispose(IndexWriter, bool)"/> was called with
/// <c>false</c>. Normally this exception is
/// privately caught and suppresed by <see cref="IndexWriter"/>.
/// </summary>
Expand Down

0 comments on commit a26bd2b

Please sign in to comment.