NightOwl888 commented on code in PR #1074:
URL: https://github.com/apache/lucenenet/pull/1074#discussion_r1896021419
##########
src/Lucene.Net.Analysis.Kuromoji/Dict/BinaryDictionary.cs:
##########
@@ -101,8 +101,8 @@ protected BinaryDictionary()
ByteBuffer buffer; // LUCENENET: IDE0059: Remove unnecessary value
assignment
using (Stream mapIS = GetResource(TARGETMAP_FILENAME_SUFFIX))
+ using (var @in = new InputStreamDataInput(mapIS)) // LUCENENET:
CA2000: Use using statement
Review Comment:
What is effectively happening here is that `mapIS` is having `Dispose()`
called on it twice.
So, looking at the other issues here, it looks like
`InputStreamDataInput`/`OutputStreamDataOutput` (and anything that inherits
them) should be given a set of constructor overloads that have a `leaveOpen`
parameter. We have done this elsewhere to match this convention of .NET. That
would allow for some flexibility to allow the caller to take charge of the
disposal, if needed.
In this case (and all other usages of these 2 classes in this PR) we could
set it to `leaveOpen: true` in addition to adding the `using` block to satisfy
the CA2000 analyzer without causing differing behavior.
Given that we still have types that use the decorator pattern around
`Stream`/`TextReader`/`TextWriter` and similar subclasses that don't have a
`leaveOpen` parameter, we should do a review to determine if there are any
others that would make good candiates for adding `leaveOpen`. Note that
`IndexInput`/`IndexOutput` subclasses might be good candidates for this
pattern, as it would allow us to call `Dispose() on them while not calling it
on the class that they wrap.
##########
src/Lucene.Net/Store/LockStressTest.cs:
##########
@@ -144,8 +144,8 @@ public static void Main(string[] args)
socket.Connect(verifierHost, verifierPort);
using Stream stream = new NetworkStream(socket);
- BinaryReader intReader = new BinaryReader(stream);
- BinaryWriter intWriter = new BinaryWriter(stream);
+ using BinaryReader intReader = new BinaryReader(stream);
+ using BinaryWriter intWriter = new BinaryWriter(stream);
Review Comment:
After reviewing the code of `BinaryReader` and `BinaryWriter`, this will
effectively call `Dispose()` on `stream` 3 times and they don't do anything
else other than that when calling `Dispose()`. So, technically, this was not
broken.
To get rid of the warning, I would suggest keeping the `using` and calling
the `(Stream, Encoding, bool)` constructor overloads. Actually, there is a
slight performance advantage to doing so because .NET Framework calls `new
UTF8Encoding()` instead of `Encoding.UTF8`. Pass `leaveOpen: true` for the 3rd
parameter.
##########
src/Lucene.Net.Analysis.Kuromoji/Dict/BinaryDictionary.cs:
##########
@@ -124,8 +124,8 @@ protected BinaryDictionary()
}
using (Stream posIS = GetResource(POSDICT_FILENAME_SUFFIX))
+ using (var @in = new InputStreamDataInput(posIS)) // LUCENENET:
CA2000: Use using statement
Review Comment:
See my comment above.
##########
src/Lucene.Net.Analysis.Kuromoji/Dict/CharacterDefinition.cs:
##########
@@ -61,7 +61,7 @@ private enum CharacterClass : byte
private CharacterDefinition()
{
using Stream @is = BinaryDictionary.GetTypeResource(GetType(),
FILENAME_SUFFIX);
- DataInput @in = new InputStreamDataInput(@is);
+ using var @in = new InputStreamDataInput(@is); // LUCENENET:
CA2000: Use using statement
Review Comment:
See my comment in `BinaryDictionary`.
##########
src/Lucene.Net.Analysis.Kuromoji/Tools/CharacterDefinitionWriter.cs:
##########
@@ -80,7 +80,7 @@ public void Write(string baseDir)
//new File(filename).getParentFile().mkdirs();
System.IO.Directory.CreateDirectory(System.IO.Path.GetDirectoryName(baseDir));
using Stream os = new FileStream(filename, FileMode.Create,
FileAccess.Write);
- DataOutput @out = new OutputStreamDataOutput(os);
+ using var @out = new OutputStreamDataOutput(os); // LUCENENET:
CA2000: Use using statement
Review Comment:
See my comment in `BinaryDictionary`.
##########
src/Lucene.Net.Analysis.Kuromoji/Dict/BinaryDictionary.cs:
##########
@@ -151,9 +151,9 @@ protected BinaryDictionary()
ByteBuffer tmpBuffer;
using (Stream dictIS = GetResource(DICT_FILENAME_SUFFIX))
+ // no buffering here, as we load in one large buffer
+ using (var @in = new InputStreamDataInput(dictIS)) // LUCENENET:
CA2000: Use using statement
{
Review Comment:
See my comment above.
##########
src/Lucene.Net/Store/LockVerifyServer.cs:
##########
@@ -134,8 +134,8 @@ public ThreadAnonymousClass(object localLock, int[]
lockedID, CountdownEvent sta
public override void Run()
{
using Stream stream = new NetworkStream(cs);
- BinaryReader intReader = new BinaryReader(stream);
- BinaryWriter intWriter = new BinaryWriter(stream);
+ using BinaryReader intReader = new BinaryReader(stream);
+ using BinaryWriter intWriter = new BinaryWriter(stream);
Review Comment:
After reviewing the code of `BinaryReader` and `BinaryWriter`, this will
effectively call `Dispose()` on `stream` 3 times and they don't do anything
else other than that when calling `Dispose()`. So, technically, this was not
broken.
To get rid of the warning, I would suggest keeping the `using` and calling
the `(Stream, Encoding, bool)` constructor overloads. Actually, there is a
slight performance advantage to doing so because .NET Framework calls `new
UTF8Encoding()` instead of `Encoding.UTF8`. Pass `leaveOpen: true` for the 3rd
parameter.
##########
src/Lucene.Net.Tests.Suggest/Suggest/Fst/LargeInputFST.cs:
##########
@@ -32,30 +32,31 @@ public static class LargeInputFST // LUCENENET specific:
CA1052 Static holder ty
// LUCENENET specific - renaming from Main() because we must only have
1 entry point.
// Not sure why this utility is in a test project anyway - this seems
like something that should
// be in Lucene.Net.Suggest so we can put it into the lucene-cli tool.
- public static void Main2(string[] args)
+ public static void Main2(string[] args)
{
FileInfo input = new FileInfo("/home/dweiss/tmp/shuffled.dict");
- int buckets = 20;
- int shareMaxTail = 10;
+ const int buckets = 20;
+ const int shareMaxTail = 10;
ExternalRefSorter sorter = new ExternalRefSorter(new
OfflineSorter());
FSTCompletionBuilder builder = new FSTCompletionBuilder(buckets,
sorter, shareMaxTail);
- TextReader reader =
- new StreamReader(
- new FileStream(input.FullName, FileMode.Open),
Encoding.UTF8);
-
- BytesRef scratch = new BytesRef();
- string line;
- int count = 0;
- while ((line = reader.ReadLine()) != null)
+ // LUCENENET specific - dispose of fileStream and reader when done
+ using (FileStream fileStream = new FileStream(input.FullName,
FileMode.Open))
+ using (TextReader reader = new StreamReader(fileStream,
Encoding.UTF8))
Review Comment:
Lets pass `leaveOpen: true` here. Although, it doesn't matter because
according to the comment this is basically dead code.
##########
src/Lucene.Net.TestFramework/Util/TestUtil.cs:
##########
@@ -201,8 +201,9 @@ public static void CheckReader(IndexReader reader)
public static void CheckReader(AtomicReader reader, bool
crossCheckTermVectors)
{
- ByteArrayOutputStream bos = new ByteArrayOutputStream(1024);
- StreamWriter infoStream = new StreamWriter(bos, Encoding.UTF8);
+ // LUCENENET: dispose the StreamWriter and ByteArrayOutputStream
when done
+ using ByteArrayOutputStream bos = new ByteArrayOutputStream(1024);
+ using StreamWriter infoStream = new StreamWriter(bos,
Encoding.UTF8);
Review Comment:
It looks like the proper way to fix this to pass the code analyzer would be
to pass the `leaveOpen: true` argument to the `StreamReader` constructor,
otherwise the stream will be disposed twice.
##########
src/Lucene.Net/Util/Constants.cs:
##########
@@ -178,9 +178,10 @@ private static string GetFramework45PlusFromRegistry()
{
const string subkey = @"SOFTWARE\Microsoft\NET Framework
Setup\NDP\v4\Full\";
- // As an alternative, if you know the computers you will query are
running .NET Framework 4.5
+ // As an alternative, if you know the computers you will query are
running .NET Framework 4.5
// or later, you can use:
- using RegistryKey ndpKey =
RegistryKey.OpenBaseKey(RegistryHive.LocalMachine,
RegistryView.Registry32).OpenSubKey(subkey);
Review Comment:
Given that this is copied from an official source
(https://learn.microsoft.com/en-us/dotnet/framework/migration-guide/how-to-determine-which-versions-are-installed#query-the-registry-using-code),
it seems like it would be a good idea to report this issue to Microsoft. This
does appear to be a real resource leak that needs to be addressed. And it
probably exists in a lot of different codebases because of the omission from
the documentation.
##########
src/Lucene.Net.Analysis.Kuromoji/Tools/BinaryDictionaryWriter.cs:
##########
@@ -298,7 +298,7 @@ protected virtual void WriteTargetMap(string filename)
//new File(filename).getParentFile().mkdirs();
System.IO.Directory.CreateDirectory(System.IO.Path.GetDirectoryName(filename));
using Stream os = new FileStream(filename, FileMode.Create,
FileAccess.Write);
- DataOutput @out = new OutputStreamDataOutput(os);
+ using var @out = new OutputStreamDataOutput(os); // LUCENENET:
CA2000: Use using statement
Review Comment:
See my comment in `BinaryDictionary`.
##########
src/Lucene.Net.Analysis.Kuromoji/Dict/ConnectionCosts.cs:
##########
@@ -39,8 +39,8 @@ private ConnectionCosts()
short[][] costs = null;
using (Stream @is = BinaryDictionary.GetTypeResource(GetType(),
FILENAME_SUFFIX))
+ using (var @in = new InputStreamDataInput(@is)) // LUCENENET:
CA2000: Use using statement
Review Comment:
See my comment in `BinaryDictionary`.
##########
src/Lucene.Net.Analysis.Kuromoji/Tools/ConnectionCostsBuilder.cs:
##########
@@ -31,7 +31,7 @@ public static class ConnectionCostsBuilder // LUCENENET
specific: CA1052 Static
public static ConnectionCostsWriter Build(string filename)
{
using Stream inputStream = new FileStream(filename, FileMode.Open,
FileAccess.Read);
- StreamReader streamReader = new StreamReader(inputStream,
Encoding.ASCII);
+ using StreamReader streamReader = new StreamReader(inputStream,
Encoding.ASCII); // LUCENENET: CA2000: Use using statement
Review Comment:
It looks like the proper way to fix this to pass the code analyzer would be
to pass the `leaveOpen: true` argument to the `StreamReader` constructor,
otherwise the stream will be disposed twice.
##########
src/Lucene.Net.Analysis.Kuromoji/Tools/BinaryDictionaryWriter.cs:
##########
@@ -355,7 +355,7 @@ protected virtual void WriteDictionary(string filename)
//new File(filename).getParentFile().mkdirs();
System.IO.Directory.CreateDirectory(System.IO.Path.GetDirectoryName(filename));
using Stream os = new FileStream(filename, FileMode.Create,
FileAccess.Write);
- DataOutput @out = new OutputStreamDataOutput(os);
+ using var @out = new OutputStreamDataOutput(os); // LUCENENET:
CA2000: Use using statement
Review Comment:
See my comment in `BinaryDictionary`.
##########
src/Lucene.Net.Analysis.Kuromoji/Tools/ConnectionCostsWriter.cs:
##########
@@ -54,11 +54,11 @@ public void Write(string baseDir)
// LUCENENET specific: we don't need to do a "classpath" output
directory, since we
// are changing the implementation to read files dynamically
instead of making the
// user recompile with the new files.
- string filename = System.IO.Path.Combine(baseDir,
typeof(ConnectionCosts).Name + CharacterDefinition.FILENAME_SUFFIX);
+ string filename = System.IO.Path.Combine(baseDir,
nameof(ConnectionCosts) + CharacterDefinition.FILENAME_SUFFIX);
//new File(filename).getParentFile().mkdirs();
System.IO.Directory.CreateDirectory(System.IO.Path.GetDirectoryName(filename));
using Stream os = new FileStream(filename, FileMode.Create,
FileAccess.Write);
- DataOutput @out = new OutputStreamDataOutput(os);
+ using var @out = new OutputStreamDataOutput(os); // LUCENENET:
CA2000: Use using statement
Review Comment:
See my comment in `BinaryDictionary`.
##########
src/Lucene.Net.Analysis.Kuromoji/Tools/ConnectionCostsWriter.cs:
##########
@@ -54,11 +54,11 @@ public void Write(string baseDir)
// LUCENENET specific: we don't need to do a "classpath" output
directory, since we
// are changing the implementation to read files dynamically
instead of making the
// user recompile with the new files.
- string filename = System.IO.Path.Combine(baseDir,
typeof(ConnectionCosts).Name + CharacterDefinition.FILENAME_SUFFIX);
+ string filename = System.IO.Path.Combine(baseDir,
nameof(ConnectionCosts) + CharacterDefinition.FILENAME_SUFFIX);
Review Comment:
This seems to be a systemic issue. Let's open an issue to fix all of these
in one PR.
##########
src/Lucene.Net.Analysis.Kuromoji/Tools/BinaryDictionaryWriter.cs:
##########
@@ -328,7 +328,7 @@ protected virtual void WritePosDict(string filename)
//new File(filename).getParentFile().mkdirs();
System.IO.Directory.CreateDirectory(System.IO.Path.GetDirectoryName(filename));
using Stream os = new FileStream(filename, FileMode.Create,
FileAccess.Write);
- DataOutput @out = new OutputStreamDataOutput(os);
+ using var @out = new OutputStreamDataOutput(os); // LUCENENET:
CA2000: Use using statement
Review Comment:
See my comment in `BinaryDictionary`.
##########
src/Lucene.Net.Analysis.Kuromoji/Tools/TokenInfoDictionaryBuilder.cs:
##########
@@ -72,7 +72,7 @@ public virtual TokenInfoDictionaryWriter
BuildDictionary(IList<string> csvFiles)
{
using Stream inputStream = new FileStream(file, FileMode.Open,
FileAccess.Read);
Encoding decoder = Encoding.GetEncoding(encoding);
- TextReader reader = new StreamReader(inputStream, decoder);
+ using TextReader reader = new StreamReader(inputStream,
decoder); // LUCENENET: CA2000: Use using statement
Review Comment:
It looks like the proper way to fix this to pass the code analyzer would be
to pass the `leaveOpen: true` argument to the `StreamReader` constructor,
otherwise the stream will be disposed twice.
--
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-unsubscr...@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
