API: Review to ensure IDisposable is being used correctly and disposable pattern implemented correctly #265

clambertus · 2019-10-10T18:17:44Z

There have been several issues found recently with the disposable pattern not being implemented correctly. I have also been made aware that there is at least one class (Lucene.Net.Store.Lock, if I recall correctly) that is designed to be re-opened after it is closed.

We need a review to ensure all classes that implement disposable are doing it correctly and have correctly implemented the dispose pattern both for sealed and unsealed types. We also need to have a close look at whether any classes should be reverted back to using Close() instead of Dispose() on account that the class instance was designed to be used again after the Dispose() call.

We should also explicitly check to ensure that Dispose() is set up to be called more than once safely. That is, after the first call, all additional calls should be a no-op.

JIRA link - [LUCENENET-626] created by nightowl888

The text was updated successfully, but these errors were encountered:

clambertus · 2019-10-10T18:22:05Z

It is believed that IncrementToken() was being called after Dispose() because of a bug in the test framework that has been addressed. We need verification that this issue is no longer happening.

by nightowl888

…. Partially addresses apache#265.

* BUG: Lucene.Net.Document.CompressionTools: Pass the offset and length to the underlying MemoryStream * BUG: Lucene.Net.Analysis.Common.Tartarus.Snowball.Among: Fixed MethodObject property to reference private field instead of itself * BUG: Lucene.Net.TestFramework.Analysis.MockTokenizer: Pass the AttributeFactory argument that is provided as per the documentation comment. Note this bug exists in Lucene 4.8.0, also. * TestTargetFramework.props: Disable IDE0052 and IDE0060 in test projects * lucene-cli: Disabled IDE0060 warnings * Lucene.Net.Demo: Disabled IDE0060 warnings * PERFORMANCE: Fixes IDE0052 Remove unread private member, fixes IDE0060 Remove unused parameter * BUG: Lucene.Net.Util.ExceptionExtensions.GetSuppressedAsList(): Use J2N.Collections.Generic.List<T> so the call to ToString() will automatically list the exception messages * Lucene.Net.Tests.Support.TestApiConsistency: Added exclusion for System.Runtime.CompilerServices to skip our nullable attribute replacements for platforms that don't support them * Fixes CA2213 Disposable fields should be disposed (except for IndexWriter and subclasses which need more work) * Fixes CA1063: Implement IDisposable Properly (except for IndexWriter). Partially addresses #265. * PERFORMANCE: Lucene.Net.Index: Changed FieldInfos, FreqProxTermsWriterPerField, IndexWriter, LogMergePolicy, SegmentCoreReaders, and SegmentReader to take advantage of the fact that TryGetValue returns a boolean * PERFORMANCE: Lucene.Net.Index.DocValuesProducer: Optimized checks in AddXXXField() methods * Fixes IDE0018: Inline variable declaration * Fixes CA1507: Use nameof in place of string * BUG: Fixed formatting in ArgumentException message for all analyzer factories so it will display the dictionary contents * Removed pattern matching warning exclusions for IDE0019, IDE0020, and IDE0038 * Fixes IDE0019: Use pattern matching to avoid 'is' check followed by a cast * Simplified reuse logic of TermsEnum classes * Fixes IDE0038: Use pattern matching to avoid 'is' check followed by a cast * Fixes IDE0020: Use pattern matching to avoid 'is' check followed by a cast * Fixes CA1802: Use Literals Where Appropriate * TestTargetFramework.props: Disabled IDE0044 warnings in test projects * TestTargetFramework.props: Disabled IDE0019, IDE0020, IDE0038 from tests * TestTargetFramework.props: Added CA1822 and IDE0059 and changed list to be in lexographical order by ID * TestTargetFramework.props: Disabled IDE1006 (Naming rule violation) warnings in tests * TestTargetFraework.props: Disabled CA1034 (Nested types should not be visible) warnings in tests * TestTargetFramework.props: Disabled CA1825 (Avoid zero-length array allocations) warnings in tests * TestTargetFramework.props: Disabled IDE0017 and IDE0028 warnings in tests (object and collection initialization can be simplified) * TestTargetFramework.props: Disabled IDE0051 (Remove unused private member) warnings in tests * Fixes IDE0044: Add readonly modifier * Directory.Build.props: Disable IDE0032 warnings (use auto property) * Fixes CA1813: Avoid unsealed attributes * PERFORMANCE: Fixes CA1822: Mark members as static * Added aggressive inlining to codecs, asserts and several other areas * Removed InitializeInstanceFields() methods in all cases except in NumericTokenStream where we need to initialize from multiple constructors * Lucene.Net.Util: Added aggressive inlining on several methods * Fixes IDE0059: Remove unnecessary value assignment * Fixes IDE0034: Simplify 'default' expression * Fixes IDE0051: Remove unused private member. Also removed dead code/commented code. * Directory.Build.props: Disabled CA1031 warnings (do not catch general exception types) * lucene-cli: Disabled CA1034 (Nested types should not be visible) warnings in tests * Fixes CA1819: Properties should not return arrays * Fixes CA1012: Abstract types should not have constructors * PERFORMANCE: Fixes CA1825: Avoid zero-length array allocations * Fixes CA1052: Static holder types should be Static or NotInheritable * Fixes IDE0028: Use collection initializers * Fixes IDE1006: Naming Styles * Fixes IDE0063: Use simple 'using' statement * Fixes IDE0016: Use throw expression * TestTargetFramework.props: Disabled IDE0031 (Use null propagation) warnings in tests * Fixes IDE0031: Use null propagation * Fixes IDE0029: Use coalesce expression * Fixes IDE0030: Use coalesce expression (nullable) * Fixes CA1820: Test for empty strings using string length * Fixes CA1810: Initialize reference type static fields inline * TestTargetFramework.props: Disabled CA2219 (Do not raise exceptions in exception clauses) warnings in tests * Directory.Build.props: Disabled IDE0070 (Use 'System.HashCode.Combine') warnings because this functionality requires .NET Standard 2.1 * Fixes IDE0025: Use expression body for properties * Fixes IDE0021: Use block body for constructors * TestTargetFramework.props: Disabled IDE0040 (Add accessibility modifiers) warnings in tests * Fixes IDE0040: Add accessibility modifiers * Fixes IDE0041: Use is null check * Fixes CA1815: Override equals and operator equals on value types * TestTargetFramework.props: Disabled IDE0049 (Use language keywords instead of framework type names for type references) warnings in tests * Fixes IDE0049: Use language keywords instead of framework type names for type references * Fixes IDE0039: Use local function * Fixes IDE0071: Simplify interpolation * Fixes IDE0027: Use expression body for accessors * fixes IDE1005: Use conditional delegate call * Added .editorconfig file to control formatting and code analysis rules * Directory.Build.props: Removed all Code Analysis settings that are now controlled by .editorconfig

nikcio · 2022-10-15T19:34:37Z

Issues found: https://sonarcloud.io/project/issues?resolved=false&rules=csharpsquid%3AS3881&id=apache_lucenenet

Mentioned in #648 :
Note this is part of #265. Most of the issues I have spotted were failure to call base.Dispose(disposing) after exiting the if (disposing) block. Others are because they are on a private class that should be sealed, but it is not (Enumerators).

…there is no method signature conflict between the public Dispose(waitForMerges) method and the protected Dispose(disposing) method. See apache#265.

…there is no method signature conflict between the public Dispose(waitForMerges) method and the protected Dispose(disposing) method that can be overridden and called from a finalizer. See apache#265.

…there is no method signature conflict between the public Dispose(waitForMerges) method and the protected Dispose(disposing) method that can be overridden and called from a finalizer. See #265.

…n Dispose() is called multiple times. Also added checks to ensure an ObjectDisposedException is thrown by any method call after Dispose() is called. Fixed all other directory implementations and made the check atomic so only the first call to Dispose() is run. Fixes apache#841. Related to apache#265.

…FSDirectory): Upgraded all FSDirectories to allow multiple Dispose() calls on IndexInputSlicer and BufferedIndexInput subclasses. See apache#265.

…s and guard against usage after Dispose(). See apache#265.

…guard against usage after Dispose(). See apache#265.

…nd guard against usage after Dispose(). See apache#265.

… guard against usage after Dispose(). See apache#265.

…nd guard against usage after Dispose(). See apache#265.

…n Dispose() is called multiple times. Also added checks to ensure an ObjectDisposedException is thrown by any method call after Dispose() is called. Fixed all other directory implementations and made the check atomic so only the first call to Dispose() is run. Fixes #841. Related to #265.

…FSDirectory): Upgraded all FSDirectories to allow multiple Dispose() calls on IndexInputSlicer and BufferedIndexInput subclasses. See #265.

…s and guard against usage after Dispose(). See #265.

…guard against usage after Dispose(). See #265.

…nd guard against usage after Dispose(). See #265.

… guard against usage after Dispose(). See #265.

…nd guard against usage after Dispose(). See #265.

paulirwin · 2024-11-25T21:51:51Z

From @stesee in #615 (comment) (moving over to this discussion)

Missing dispose calls can also be found by the NETAnalyzers that integrate into your local dev env. Are you interested in activating them in the whole solution?

Seems like there are a lot more missing dispose calls - 28 CA2000 warnings in the main project "Lucene.Net".

I think that's possibly a great idea. We'd want to enable them on more than just net8.0, if possible, and ensure this works with our CI builds. But I think any automated analysis we can do is worthwhile. I'd like @NightOwl888's thoughts too.

* Add using statements where possible, per CA2000, #265 * Fix file handle leaks in demo code, #615 * Add leaveOpen parameter to InputStreamDataInput, #265 * Add leaveOpen parameter to OutputStreamDataOutput, #265

#265 (#1095) * IDisposable Pattern Cleanup, #265 * Fix default StreamReader buffer size for .NET Framework compat

clambertus added Lucene.Net Core up-for-grabs This issue is open to be worked on by anyone labels May 5, 2020

NightOwl888 added this to the Lucene.NET 4.8.0 milestone May 7, 2020

NightOwl888 mentioned this issue Jul 11, 2020

TokenStream.IncrementToken() is called after Dispose() is called #271

Open

NightOwl888 added the pri:high label Oct 6, 2020

NightOwl888 added a commit to NightOwl888/lucenenet that referenced this issue Nov 10, 2020

Fixes CA1063: Implement IDisposable Properly (except for IndexWriter)…

0dce38e

…. Partially addresses apache#265.

NightOwl888 mentioned this issue Dec 13, 2021

When is the release version planned? #437

Closed

This was referenced Feb 3, 2022

File Handle Leak in SearchFiles demo and LargeInputFST test code #615

Closed

Cannot access the cfs file. #616

Closed

NightOwl888 mentioned this issue Oct 15, 2022

Which kinds of PR's would you like to get? #648

Closed

nikcio mentioned this issue Nov 6, 2022

fix: Aligned disposable patterns #746

Merged

NightOwl888 mentioned this issue Nov 7, 2022

BREAKING: Lucene.Net.Index.IndexWriter: Fixed Dispose(waitForMerges) vs Dispose(disposing) #749

Merged

NightOwl888 added a commit to NightOwl888/lucenenet that referenced this issue May 14, 2023

Lucene.Net.Codecs.MultiLevelSkipListReader: Allow double-dispose call…

dd96404

…s and guard against usage after Dispose(). See apache#265.

NightOwl888 added a commit to NightOwl888/lucenenet that referenced this issue May 14, 2023

Lucene.Net.Store.BufferedIndexOutput: Allow double-dispose calls and …

0321896

…guard against usage after Dispose(). See apache#265.

NightOwl888 added a commit to NightOwl888/lucenenet that referenced this issue May 14, 2023

Lucene.Net.Store.RateLimitedIndexOutput: Allow double-dispose calls a…

a846ed9

…nd guard against usage after Dispose(). See apache#265.

NightOwl888 added a commit to NightOwl888/lucenenet that referenced this issue May 14, 2023

Lucene.Net.Store.InputStreamDataInput: Allow double-dispose calls and…

c47b3ac

… guard against usage after Dispose(). See apache#265.

NightOwl888 added a commit to NightOwl888/lucenenet that referenced this issue May 14, 2023

Lucene.Net.Store.OutputStreamDataOutput: Allow double-dispose calls a…

d8b08d8

…nd guard against usage after Dispose(). See apache#265.

NightOwl888 mentioned this issue May 14, 2023

SWEEP: Lucene.Net.Store: Fixed several Dispose() methods so they are safe to be called multiple times #854

Merged

NightOwl888 added a commit that referenced this issue May 16, 2023

Lucene.Net.Store (Directory + MMapDirectory + NIOFSDirectory + Simple…

c49fd9a

…FSDirectory): Upgraded all FSDirectories to allow multiple Dispose() calls on IndexInputSlicer and BufferedIndexInput subclasses. See #265.

NightOwl888 added a commit that referenced this issue May 16, 2023

Lucene.Net.Codecs.MultiLevelSkipListReader: Allow double-dispose call…

407fe16

…s and guard against usage after Dispose(). See #265.

NightOwl888 added a commit that referenced this issue May 16, 2023

Lucene.Net.Store.BufferedIndexOutput: Allow double-dispose calls and …

3979c4a

…guard against usage after Dispose(). See #265.

NightOwl888 added a commit that referenced this issue May 16, 2023

Lucene.Net.Store.RateLimitedIndexOutput: Allow double-dispose calls a…

a9c8388

…nd guard against usage after Dispose(). See #265.

NightOwl888 added a commit that referenced this issue May 16, 2023

Lucene.Net.Store.InputStreamDataInput: Allow double-dispose calls and…

5336dfc

… guard against usage after Dispose(). See #265.

NightOwl888 added a commit that referenced this issue May 16, 2023

Lucene.Net.Store.OutputStreamDataOutput: Allow double-dispose calls a…

e9c47b2

…nd guard against usage after Dispose(). See #265.

NightOwl888 removed the Lucene.Net Core label Nov 6, 2024

paulirwin modified the milestones: 4.8.0, 4.8.0-beta00018 Nov 18, 2024

paulirwin self-assigned this Dec 21, 2024

paulirwin mentioned this issue Dec 21, 2024

Make Lucene.Net.Store.Lock reusable #1073

Open

1 task

paulirwin added a commit to paulirwin/lucene.net that referenced this issue Dec 22, 2024

Add using statements where possible, per CA2000, apache#265

f74f39c

paulirwin mentioned this issue Dec 22, 2024

Dispose of some disposables, #265, #615 #1074

Merged

4 tasks

paulirwin added a commit to paulirwin/lucene.net that referenced this issue Dec 25, 2024

Add leaveOpen parameter to InputStreamDataInput, apache#265

ef23995

paulirwin added a commit to paulirwin/lucene.net that referenced this issue Dec 25, 2024

Add leaveOpen parameter to OutputStreamDataOutput, apache#265

86e18f0

paulirwin added a commit to paulirwin/lucene.net that referenced this issue Jan 12, 2025

IDisposable Pattern Cleanup, apache#265

9473cbe

paulirwin mentioned this issue Jan 12, 2025

IDisposable Pattern Cleanup and fix TestDictionary.TestResourceCleanup, #265 #1095

Merged

4 tasks

NightOwl888 pushed a commit that referenced this issue Jan 12, 2025

IDisposable Pattern Cleanup and fix TestDictionary.TestResourceCleanup,

ad5d70a

#265 (#1095) * IDisposable Pattern Cleanup, #265 * Fix default StreamReader buffer size for .NET Framework compat

NightOwl888 closed this as completed in #1095 Jan 12, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API: Review to ensure IDisposable is being used correctly and disposable pattern implemented correctly #265

API: Review to ensure IDisposable is being used correctly and disposable pattern implemented correctly #265

clambertus commented Oct 10, 2019 •

edited by NightOwl888

Loading

clambertus commented Oct 10, 2019

nikcio commented Oct 15, 2022 •

edited

Loading

paulirwin commented Nov 25, 2024

API: Review to ensure IDisposable is being used correctly and disposable pattern implemented correctly #265

API: Review to ensure IDisposable is being used correctly and disposable pattern implemented correctly #265

Comments

clambertus commented Oct 10, 2019 • edited by NightOwl888 Loading

clambertus commented Oct 10, 2019

nikcio commented Oct 15, 2022 • edited Loading

paulirwin commented Nov 25, 2024

clambertus commented Oct 10, 2019 •

edited by NightOwl888

Loading

nikcio commented Oct 15, 2022 •

edited

Loading