[GitHub] [lucenenet] NightOwl888 commented on issue #615: File Handle Leak

[GitBox <gi...@apache.org>]

NightOwl888 commented on issue #615:
URL: https://github.com/apache/lucenenet/issues/615#issuecomment-1028718924


   Thanks for the report.
   
   Technically, this is a duplicate of #265. We haven't yet reviewed the `IDisposable` usages to ensure they are correct from a .NET perspective. Although, that issue is up-for-grabs if you are interested in contributing, and **we welcome your assistance**. We have largely saved this task for the community and have been focusing on more complicated issues that require more than just an understanding of .NET best practices to solve.
   
   Lucene.NET is (for the most part) a line-by-line port of [Lucene 4.8.0](https://github.com/apache/lucene/tree/releases/lucene-solr%2F4.8.0/lucene). As such, we carried over many of the gaps between Java and .NET. One such gap is that in Java the [Closable interface](https://docs.oracle.com/javase/8/docs/api/java/io/Closeable.html) inherits the [AutoCloseable interface](https://docs.oracle.com/javase/8/docs/api/java/lang/AutoCloseable.html), which can be configured to automatically be cleaned up. We haven't yet researched whether Lucene is configured with that option enabled, but suspect that is the reason why the code doesn't explicitly call `.close()` (and why the .NET version doesn't explicitly call `.Dispose()`). This includes the two [you mentioned](https://github.com/apache/lucenenet/issues/615#issue-1122670124).
   
   Given this fact, there are likely lots of places where there are missing calls to `Dispose()`, most of which can be added (with a comment `LUCENENET specific: added call to Dispose()` or `LUCENENET specific: added using block`).
   
   Note there is at least 1 known place where the call to `Dispose()` is problematic in #271. We are seeing multiple calls to `Dispose()` because we have added [this line in `Tokenizer`](https://github.com/apache/lucenenet/blob/972d1f50dcb7b49cd08ae91cd984309f942909b8/src/Lucene.Net/Analysis/Tokenizer.cs#L76) that [didn't exist in Lucene](https://github.com/apache/lucene/blob/releases/lucene-solr/4.8.0/lucene/core/src/java/org/apache/lucene/analysis/Tokenizer.java#L65-L70). I suspect that Lucene falls back to the `AutoCloseable` config in this case, and the closest .NET equivalent is to use a [finalizer](https://techcommunity.microsoft.com/t5/iis-support-blog/understanding-when-to-use-a-finalizer-in-your-net-class/ba-p/347760), but both of these assumptions need confirmation. Clearly, using a finalizer has drawbacks and we should avoid that approach and use explicit calls to `Dispose()`, if possible. It also seems like `Dispose()` and `Close()` should be separate concepts in this par
 ticular case, being that this class is expected to be reused after it is "closed" without creating a new instance. But, the bottom line is we cannot just add all of the missing `Dispose()` calls and `using` statements and assume that is the correct approach - each case needs to be considered carefully according to its existing usage in Java.
   
   It sounds like [CodeQL](https://codeql.github.com/docs/codeql-language-guides/codeql-for-csharp/) can be utilized to speed up the process of locating them. My suggestion would be to submit a separate commit for each of the issues you found, and break them down into multiple PRs in some sensible way, so we can review and test them thoroughly chunk by chunk
   
   


-- 
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