Skip to content

Port LUCENE-5654 from Lucene 4.8.1, #1381#1424

Draft
paulirwin wants to merge 16 commits into
apache:masterfrom
paulirwin:issue/1381-batch-5
Draft

Port LUCENE-5654 from Lucene 4.8.1, #1381#1424
paulirwin wants to merge 16 commits into
apache:masterfrom
paulirwin:issue/1381-batch-5

Conversation

@paulirwin

Copy link
Copy Markdown
Contributor
  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a change, please open an issue 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). 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.

@paulirwin paulirwin added the notes:breaking-change Has changes that will break backward compatibility label Jul 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

notes:breaking-change Has changes that will break backward compatibility

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant