fix: Aligned disposable patterns

[nikcio]

Related to #265

I've run through most of the issues reported in Sonarcloud related to the use of the IDisposable pattern (https://sonarcloud.io/project/issues?resolved=false&rules=csharpsquid%3AS3881&id=apache_lucenenet)

This PR is meant to further expose which implementations of the pattern require more investigation while also removing the easiest areas where the pattern just needs a simple alignment. This means that if any of the following changes exposes an issue requiring much more effort this is best to be moved to comment on the existing issue and have a solution opened in another PR. I realize that a correct implementation of this pattern can be advanced and is related to at least a handful of issues opened in this repository which is why I want to take the approach stated 😄 .

To help the review I've outlined the changes here in some categories:

Issues from private classes were marked sealed

Private classes don't have to uphold the same pattern as other classes if they are marked sealed.

Basic implementation of pattern in some classes

The following files had a class where the following template was implemented.

public void Dispose()
{
    Dispose(true);
    GC.SuppressFinalize(this);
}

protected virtual void Dispose(bool disposing)
{
    if (disposing)
    {
        // Cleanup
    }
}

1. BufferedCharFilter
2. MemoryDocValuesConsumer
3. PerFieldDocValuesFormat
4. PerFieldPostingsFormat
5. IndexWriter
6. TaskMergeScheduler

base.Dispose(disposing) referencing code

The base.Dispose(disposing) line in BufferedCharFilter references the dispose method in Lucene.Net.Analysis.CharFilter

base.Dispose(disposing) referencing disposing methods in System.IO

The changes in the following files have base.Dispose(disposing) methods which reference code in System.IO

1. SourceCodeSectionReader
2. SafeTextWriterWrapper

Fixed GC.SuppressFinalize call

In PrefixCodedTerms the call to GC.SuppressFinalize had the object parameter set to true instead of this

FSDirectory

There's a commented base.Disposing(disposed) change in FSDirectory which when activated causes at least 140 tests to fail (I didn't run it all the way through when I noticed the error). Please validate that the Dispose(bool) method isn't supposed to call its base.

Other base.Dispose(dispoing)

Other places that aren't part of the categories above reference an empty Dispose(bool) method in its base. Therefore, this shouldn't cause any side effects and only make future development more reliable as the classes will follow the same dispose pattern.

Sidenote

Assuming the comment around this Dispose method is correct this issue can be removed as intended either in SonarCloud or in source: https://sonarcloud.io/project/issues?issues=AYRH0T17_qq9ReJdi40Q&open=AYRH0T17_qq9ReJdi40Q&id=apache_lucenenet

[NightOwl888]

Thanks for the PR.

The token streams are still a WIP (see #271 (comment)), but I think having them set up consistently for .NET is a good starting point even if we eventually change them back to using Close() instead of Dispose(). So, we can keep those changes in this PR.

There are just a couple of minor changes to do before this can be accepted. See the comments inline.

[NightOwl888]

Sidenote

Assuming the comment around this Dispose method is correct this issue can be removed as intended either in SonarCloud or in source: https://sonarcloud.io/project/issues?issues=AYRH0T17_qq9ReJdi40Q&open=AYRH0T17_qq9ReJdi40Q&id=apache_lucenenet

It looks like this was mainly done to prevent the end user from being able to override the default Dispose(bool) implementation and forget to call base.Dispose(disposing);. We can probably just make the protected Dispose(bool) into a virtual method and move the implementation there to address this warning as long as we include doc comments to always call the base implementation when overriding Dispose(bool).

[NightOwl888]

FSDirectory

There's a commented base.Disposing(disposed) change in FSDirectory which when activated causes at least 140 tests to fail (I didn't run it all the way through when I noticed the error). Please validate that the Dispose(bool) method isn't supposed to call its base.

I took a look and the only thing that BufferedIndexOutput does in its Dispose(bool) method is call Flush(). There is a comment at the top of FSIndexOutput that give some insight:

// LUCENENET specific: Since FileStream does its own buffering, this class was refactored
// to do all checksum operations as well as writing to the FileStream. By doing this we elminate
// the extra set of buffers that were only creating unnecessary memory allocations and copy operations.

So, the inheritance chain was left unchanged (for consistency with Lucene), but we aren't actually using the functionality of the base class. Instead, we call Flush() on FileStream and dispose it.

The BufferedIndexOutput class doesn't actually have any IDisposable objects in it.

This looks fine.

[NightOwl888]

base.Dispose(disposing) referencing code

The base.Dispose(disposing) line in BufferedCharFilter references the dispose method in Lucene.Net.Analysis.CharFilter

FYI - The BufferedCharFilter is a patch to fix TextReader, since .NET doesn't have a BufferedTextReader. So, we decorate a TextReader with this class in order to make it support the extra state and methods to make it "buffered".

[NightOwl888]

@nikcio - Thanks again for doing this. Your help is really appreciated.

I went ahead and updated the comments to finish this up (for the most part). We will know what else needs to be done in the scan, and we are tracking #265 and we also have a plan to fix #271.

I didn't do anything with TaxononmyReader yet. We need to get to the bottom of why we are seeing locking contention in the tests that didn't exist in Lucene before tackling that one.