[PR] Port LUCENE-5654 from Lucene 4.8.1, #1381 [lucenenet]

["paulirwin (via GitHub)" <gi...@apache.org>]

paulirwin opened a new pull request, #1424:
URL: https://github.com/apache/lucenenet/pull/1424

   - [X] You've read the [Contributor Guide](https://github.com/apache/lucenenet/blob/master/CONTRIBUTING.md) and [Code of Conduct](https://www.apache.org/foundation/policies/conduct.html).
   - [X] You've included unit or integration tests for your change, where applicable.
   - [X] You've included inline docs for your change, where applicable.
   - [X] There's an open issue for the PR that you are making. If you'd like to propose a change, please [open an issue](https://github.com/apache/lucenenet/issues/new/choose) to discuss the change or find an existing issue.
   
   Port LUCENE-5654 from Lucene 4.8.1
   
   Partial #1381 (Batch 5)
   
   ## Description
   
   DO NOT MERGE: This PR is a stacked PR on #1413. Do not merge until that is merged.
   
   This ports LUCENE-5654 which cleans up the suppressed exceptions on various close/dispose calls. The original changelog message in Java was: "Fix various close() methods that could suppress throwables such as OutOfMemoryError, instead returning scary messages that look like index corruption."
   
   This also ports the rest of the Cranky codec suite, and adds some tests. 
   
   DRAFT NOTE: I am still working on TestIndexWriterOutOfMemory (commit 15b41abcc) and TestIndexWriterThreadsToSegments (commit 02163a83f).
   
   For the close changes, we can't use `using` which is the .NET equivalent of Java's try-with-resources, because we made the decision to make TokenStream `ICloseable` instead of `IDisposable` so that it semantically can be "reopened." So this retains a `try`/`finally` but removes the `catch`/suppression.
   
   Note that this has a slight potential difference in behavior from Java in the event that `Close()` throws when the body also throws. However, even switching to `using`/`IDisposable` would not solve this:
   
   | Case | This PR | C# `using` | Java try-with-resources / prior to this PR |
   | --- | --- | --- | --- |
   | Body ok, Close/Dispose ok | normal completion | normal completion | normal completion |
   | Body throws A, Close/Dispose ok | A propagates | A propagates | A propagates |
   | Body ok, Close/Dispose throws B | B propagates | B propagates | B propagates |
   | Body throws A, Close/Dispose throws B | B propagates, A lost | B propagates, A lost | A propagates, B suppressed |
   
   Note that this PR is the most faithful to upstream, so I think it's worth considering the merit of not changing this. However, we could change this in a few different ways:
   
   1. Call `IOUtils.CloseWhileHandlingException(...)` instead of `.Close()`. This would result in "A propagates, B lost" which is very close to the original behavior, just with losing B. I doubt much other than logging code actually inspects those suppressed exceptions in the real world (and nothing does in Lucene).
   2. Add an `.AsDisposable()` method to `TokenStream` so that we can wrap it in a `using` block/statement. This makes the code more closely match Java's try-with-resources, but as you can see above it doesn't help the exception behavior.
   3. Revert the removal of the priorException overload and calls, while retaining the `success` logic. This would create a significant deviation from upstream at every callsite, with more to maintain. But it would let us match Java's logic.
   4. Leave this as-is (my vote). TokenStream's Close overrides don't typically throw, and it's not very common to throw from Dispose (mostly what is called on other implementations' fields from Close) in .NET, as it is discouraged (see [CA1065 as an example](https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1065)). This lets us most closely match the Java code, allowing for a possible difference in behavior in the very-rare and discouraged-design case where Close/Dispose happens to throw when there's _also_ an exception in the wrapped body.
   
   So my first choice would be option 4 (leave as-is), with option 1 (change to `CloseWhileHandlingException`) as an acceptable alternative.
   
   Also note that I had to slightly change the TestIndexWriterOnJRECrash change from #1292 to not use the now-removed priorException overload.
   
   Breaking change notice: this is a breaking change because the IOUtils method is being removed.


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

Re: [PR] Port LUCENE-5654 from Lucene 4.8.1, #1381 [lucenenet]

["paulirwin (via GitHub)" <gi...@apache.org>]

paulirwin commented on PR #1424:
URL: https://github.com/apache/lucenenet/pull/1424#issuecomment-6229245493

   @NightOwl888 Ready for review, now that #1413 has merged. This is the last PR batch of changes for #1381 to bring us up to full 4.8.1 compatibility.
   
   Since it's the last one, I had Claude Code run through every commit in Lucene between 4.8.0 and 4.8.1 one last time to see if we missed anything. It found a few minor gaps that were closed in some commits I just pushed. Once this is merged, we can officially say Lucene.NET targets Lucene 4.8.1.


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

Re: [PR] Port LUCENE-5654 from Lucene 4.8.1, #1381 [lucenenet]

["paulirwin (via GitHub)" <gi...@apache.org>]

paulirwin merged PR #1424:
URL: https://github.com/apache/lucenenet/pull/1424


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

Re: [PR] Port LUCENE-5654 from Lucene 4.8.1, #1381 [lucenenet]

["paulirwin (via GitHub)" <gi...@apache.org>]

paulirwin commented on PR #1424:
URL: https://github.com/apache/lucenenet/pull/1424#issuecomment-6497477896

   @NehanPathan Thanks for the review! I decided to shadow it instead with `new` to let the method that uses it still match the upstream name, and this matches the Java behavior of automatically shadowing it as well.


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