You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@lucenenet.apache.org by GitBox <gi...@apache.org> on 2022/11/06 19:42:31 UTC
[GitHub] [lucenenet] nikcio opened a new pull request, #746: fix: Aligned disposable patterns
nikcio opened a new pull request, #746:
URL: https://github.com/apache/lucenenet/pull/746
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.
```csharp
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [lucenenet] NightOwl888 commented on a diff in pull request #746: fix: Aligned disposable patterns
Posted by GitBox <gi...@apache.org>.
NightOwl888 commented on code in PR #746:
URL: https://github.com/apache/lucenenet/pull/746#discussion_r1014982216
##########
src/Lucene.Net/Index/PrefixCodedTerms.cs:
##########
@@ -87,7 +87,7 @@ internal PrefixCodedTermsIterator(RAMFile buffer)
public void Dispose()
{
Dispose(true);
- GC.SuppressFinalize(true);
+ GC.SuppressFinalize(this);
Review Comment:
Good catch
##########
src/Lucene.Net.Spatial/Prefix/AbstractVisitingPrefixTreeFilter.cs:
##########
@@ -367,7 +367,7 @@ protected internal virtual void Scan(int scanDetailLevel)
/// <summary>
/// Used for <see cref="VNode.children"/>.
/// </summary>
- private class VNodeCellEnumerator : IEnumerator<VNode>
+ private sealed class VNodeCellEnumerator : IEnumerator<VNode>
Review Comment:
Please add a comment to the end of this line ` // LUCENENET: Marked sealed` to indicate we intentionally diverged from Lucene here.
##########
src/Lucene.Net.Facet/Taxonomy/WriterCache/CollisionMap.cs:
##########
@@ -213,7 +213,7 @@ internal virtual int GetMemoryUsage()
return memoryUsage;
}
- private class EntryEnumerator : IEnumerator<Entry>
+ private sealed class EntryEnumerator : IEnumerator<Entry>
Review Comment:
Please add a comment to the end of this line ` // LUCENENET: Marked sealed` to indicate we intentionally diverged from Lucene here.
##########
src/Lucene.Net/Store/FSDirectory.cs:
##########
@@ -562,6 +562,7 @@ protected override void Dispose(bool disposing)
}
}
}
+ //base.Dispose(disposing); // Causes object to be disposed in some tests
Review Comment:
Could you elaborate a bit more what is happening here? Also, please prefix the comment with `// LUCENENET:` so we know that it wasn't a Lucene comment.
##########
src/Lucene.Net/Support/Index/TaskMergeScheduler.cs:
##########
@@ -621,14 +621,23 @@ private void Run(CancellationToken cancellationToken)
public void Dispose()
{
- if (_isDisposed)
+ Dispose(true);
Review Comment:
For future reference, please do not waste any time on `TaskMergeScheduler`. It has been deprecated and will be removed before the release.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [lucenenet] NightOwl888 commented on pull request #746: fix: Aligned disposable patterns
Posted by GitBox <gi...@apache.org>.
NightOwl888 commented on PR #746:
URL: https://github.com/apache/lucenenet/pull/746#issuecomment-1305623001
> ## `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:
```c#
// 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [lucenenet] NightOwl888 commented on pull request #746: fix: Aligned disposable patterns
Posted by GitBox <gi...@apache.org>.
NightOwl888 commented on PR #746:
URL: https://github.com/apache/lucenenet/pull/746#issuecomment-1312456109
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [lucenenet] NightOwl888 commented on pull request #746: fix: Aligned disposable patterns
Posted by GitBox <gi...@apache.org>.
NightOwl888 commented on PR #746:
URL: https://github.com/apache/lucenenet/pull/746#issuecomment-1305604942
> ## 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)`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [lucenenet] NightOwl888 merged pull request #746: fix: Aligned disposable patterns
Posted by GitBox <gi...@apache.org>.
NightOwl888 merged PR #746:
URL: https://github.com/apache/lucenenet/pull/746
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org