lucenenet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [lucenenet] NightOwl888 opened a new issue #446: Review exception types thrown and in try/catch blocks
Date Sat, 20 Mar 2021 04:58:12 GMT

NightOwl888 opened a new issue #446:
URL: https://github.com/apache/lucenenet/issues/446


   We need to review exception types to ensure we are throwing and catching the right exceptions.
   
   ## Why does it matter?
   
   In Lucene, exceptions are used like glorified `goto` statements for control flow by throwing exceptions in one place and catching them further up the stack. While we made some good effort to remove this behavior from most of the application, it is baked into the design of `IndexWriter`, `IndexReader` and many types that support them. Removing the behaivor would require some major reworking of the design and IMO would be too far removed from Lucene's design to be maintainable.
   
   ## How it works in Lucene
   
   Much like in .NET, Java exception families are controlled by inheritance. However, there are certain "checked" exceptions that *must* be handled or declared in the method signature for the caller to handle, and this behavior is enforced by the compiler. So, to centralize places that handle common exception scenarios, in Java it is convenient to throw an exception in many places and handle that exception in a single place.
   
   There are 4 main types of exceptions in Java and one more family that Lucene cares about (`java.io.IOException`):
   
   - `java.lang.Throwable` - Base class of all errors.
     - `java.lang.Error` - indicates serious problems that a reasonable application should not try to catch.
     - `java.lang.Exception` - indicates conditions that a reasonable application might want to catch.
       - `java.lang.RuntimeException` - `RuntimeException` and its subclasses are unchecked exceptions. Unchecked exceptions do not need to be declared in a method or constructor's throws clause if they can be thrown by the execution of the method or constructor and propagate outside the method or constructor boundary.
       - `java.io.IOException` - the general class of exceptions produced by failed or interrupted I/O operations.
   
   Basically, Java applications must handle everything that derive from `java.lang.Exception` except for exceptions that derive from `java.lang.RuntimeException`. As a result, a common thing to do throughout the Lucene codebase is to catch `java.lang.Exception`  (and all derived classes of it) and wrap it in a `java.lang.RuntimeException` so it can be re-thrown. This has a side-effect of bypassing all catch blocks and throwing the exception to the outside world.
   
   > **NOTE:** This bypass logic is hard to spot by the untrained eye. It is tempting to take the catch blocks out and let the error propagate if not considered carefully.
   
   ## How Lucene.NET is doing it
   
   Since this is basically a line-by-line port, we use the closest exception type we could match with the Java exception type. However:
   
   1. We got it wrong in several cases.
   2. We did it inconsistently - an exception type in one place wasn't translated to the same exception type in another place.
   3. We used `System.Exception` to replace `java.lang.Throwable`, `java.lang.Error`, `java.lang.Exception`, and `java.lang.RuntimeException` without considering all of the ramifications.
   4. Java and .NET do not consistently derive from the same families of exceptions, leaving several gaps when inheriting general exception types.
   
   This means we are catching exceptions in cases where we should not, and allowing exceptions to propagate that we should be catching and handling.
   
   ## Java Exceptions vs .NET Exceptions in Lucene
   
   The following table is a listing of all exception types caught in Lucene along with their inheritance hierarchy in both Java and in excpetions that translated them to in .NET. Note that the .NET type list is not comprehensive, but demonstrates that we aren't catching excpetions correctly because of the errors we made above.
   
   <details>
     <summary>Click for details</summary>
   
   
   <table>
     <tr>
       <th>Java Exception Type</th>
       <th>Java Superclass Types</th>
       <th>.NET Exception Type</th>
       <th>.NET Superclass Types</th>
       <th>Notes</th>
     </tr>
     <tr>
       <td>java.io.IOException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception</td>
       <td>System.IO.IOException</td>
       <td>System.Object <br/>- System.Exception <br/>-- System.SystemException</td>
       <td></td>
     </tr>
     <tr>
       <td rowspan="2">java.lang.ArrayIndexOutOfBoundsException</td>
       <td rowspan="2">java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception <br/>--- java.lang.RuntimeException <br/>-- java.lang.IndexOutOfBoundsException</td>
       <td>System.IndexOutOfRangeException</td>
       <td>System.Object <br/>- System.Exception <br/>-- System.SystemException</td>
       <td></td>
     </tr>
     <tr>
       <td>System.ArgumentOutOfRangeException</td>
       <td>System.Object <br/>- System.Exception <br/>-- System.SystemException<br/>--- System.ArgumentException</td>
       <td>We are currently not catching this exception in all places where IndexOutOfRangeException is caught.</td>
     </tr>
     <tr>
       <td rowspan="2">java.lang.StringIndexOutOfBoundsException</td>
       <td rowspan="2">java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception <br/>--- java.lang.RuntimeException <br/>-- java.lang.IndexOutOfBoundsException</td>
       <td>System.IndexOutOfRangeException</td>
       <td>System.Object <br/>- System.Exception <br/>-- System.SystemException</td>
       <td></td>
     </tr>
     <tr>
       <td>System.ArgumentOutOfRangeException</td>
       <td>System.Object <br/>- System.Exception <br/>-- System.SystemException<br/>--- System.ArgumentException</td>
       <td>We are currently not catching this exception in all places where IndexOutOfRangeException is caught.</td>
     </tr>
     <tr>
       <td rowspan="2">java.lang.IndexOutOfBoundsException</td>
       <td rowspan="2">java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception <br/>--- java.lang.RuntimeException</td>
       <td>System.IndexOutOfRangeException</td>
       <td>System.Object <br/>- System.Exception <br/>-- System.SystemException</td>
       <td></td>
     </tr>
     <tr>
       <td>System.ArgumentOutOfRangeException</td>
       <td>System.Object <br/>- System.Exception <br/>-- System.SystemException<br/>--- System.ArgumentException</td>
       <td>We are currently not catching this exception in all places where IndexOutOfRangeException is caught.</td>
     </tr>
     <tr>
       <td rowspan="3">java.text.ParseException</td>
       <td rowspan="3">java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception</td>
       <td>System.FormatException</td>
       <td>System.Object <br/>- System.Exception <br/>-- System.SystemException</td>
       <td>Does not explicitly support a way to keep track of the error index, but we could add an extension method and store the information in the FormatExcption.Data property. Also, in QueryParser there were ParseException classes that had been generated in Java - we might be able to eliminate these and just use FormatException consistently.</td>
     </tr>
     <tr>
       <td>System.InvalidOperationException</td>
       <td>System.Object <br/>- System.Exception <br/>-- System.SystemException</td>
       <td>There are some places where ParseException errors were translated to InvalidOperationException - these should probably be changed to FormatException.</td>
     </tr>
     <tr>
       <td>System.Exception</td>
       <td>System.Object</td>
       <td>There are some places where ParseException errors were translated to Exception - these should probably be changed to FormatException.</td>
     </tr>
     <tr>
       <td>java.lang.Exception</td>
       <td>java.lang.Object <br/>- java.lang.Throwable</td>
       <td>System.Exception</td>
       <td>System.Object</td>
       <td></td>
     </tr>
     <tr>
       <td>java.lang.CloneNotSupportedException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception</td>
       <td>None</td>
       <td></td>
       <td></td>
     </tr>
     <tr>
       <td rowspan="2">java.io.FileNotFoundException</td>
       <td rowspan="2">java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception <br/>--- java.io.IOException</td>
       <td>System.IO.FileNotFoundException</td>
       <td>System.Object <br/>- System.Exception <br/>-- System.SystemException <br/>--- System.IO.IOException</td>
       <td></td>
     </tr>
     <tr>
       <td>System.IO.DirectoryNotFoundException</td>
       <td>System.Object <br/>- System.Exception <br/>-- System.SystemException <br/>--- System.IO.IOException</td>
       <td></td>
     </tr>
     <tr>
       <td>java.lang.NoSuchMethodException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception <br/>--- java.lang.ReflectiveOperationException</td>
       <td>System.MissingMethodException</td>
       <td>System.Object <br/>- System.Exception <br/>-- System.SystemException <br/>--- System.IO.IOException</td>
       <td>This is wrong. Reflection in .NET doesn't throw exceptions when it cannot find a method, it simply returns null. However, it can throw when there is an ambiguous match, and not sure that is accounted for.</td>
     </tr>
     <tr>
       <td>java.lang.reflect.InvocationTargetException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception <br/>--- java.lang.ReflectiveOperationException</td>
       <td>System.TargetInvocationException</td>
       <td>System.Object <br/>- System.Exception <br/>-- System.ApplicationException</td>
       <td></td>
     </tr>
     <tr>
       <td rowspan="2">java.lang.IllegalAccessException</td>
       <td rowspan="2">java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception <br/>--- java.lang.ReflectiveOperationException</td>
       <td>System.Exception</td>
       <td>System.Object <br/>- System.Exception</td>
       <td>This seems to be done as a catch-all in .NET because Reflection Invoke() can throw a number of different exceptions. But this needs further review.</td>
     </tr>
     <tr>
       <td>System.UnauthorizedAccessException</td>
       <td>System.Object <br/>- System.Exception <br/>-- System.SystemException<</td>
       <td>This exception doesn't happen during Reflection calls, so this should be changed to System.MemberAccessException, although partial trust is obsolete so this error is likely only valid on .NET Framework.</td>
     </tr>
     <tr>
       <td rowspan="3">java.lang.IllegalArgumentException</td>
       <td rowspan="3">java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception <br/>--- java.lang.RuntimeException</td>
       <td>System.ArgumentException</td>
       <td>System.Object <br/>- System.Exception <br/>-- System.SystemException</td>
       <td></td>
     </tr>
     <tr>
       <td>System.ArgumentOutOfRangeException</td>
       <td>System.Object <br/>- System.Exception <br/>-- System.SystemException <br/>--- System.ArgumentException</td>
       <td>Some guard clauses have been added with this type. Need to check to ensure places that catch IndexOutOfRangeException and this exception are correct.</td>
     </tr>
     <tr>
       <td>System.ArgumentNullException</td>
       <td>System.Object <br/>- System.Exception <br/>-- System.SystemException <br/>--- System.ArgumentException</td>
       <td>Some guard clauses have been added with this type (J2N). Since it subclasses ArgumentException, we are covering IllegalArgumentException, but need to review NullReferenceException to see if there are problems and perhaps merge the two.</td>
     </tr>
     <tr>
       <td>java.lang.NullPointerException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception <br/>--- java.lang.RuntimeException</td>
       <td>System.NullReferenceException</td>
       <td>System.Object <br/>- System.Exception <br/>-- System.SystemException</td>
       <td>It might be best to convert everywhere this is used to ArgumentNullException so we don't swallow NullReferenceException unintentionally and hide problems.</td>
     </tr>
     <tr>
       <td>java.lang.InstantiationException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception <br/>--- java.lang.ReflectiveOperationException</td>
       <td>System.TypeInitializationException</td>
       <td>System.Object <br/>- System.Exception <br/>-- System.SystemException</td>
       <td>In Java, the exception comes from Reflection. In .NET, it may happen due to a class initializer that throws an uncaught exception. These 2 are definitely not the same thing, but I am not sure it makes a difference.</td>
     </tr>
     <tr>
       <td>java.lang.UnsupportedOperationException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception <br/>--- java.lang.RuntimeException</td>
       <td>System.NotSupportedException</td>
       <td>System.Object <br/>- System.Exception <br/>-- System.SystemException</td>
       <td></td>
     </tr>
     <tr>
       <td>java.lang.Throwable</td>
       <td>java.lang.Object</td>
       <td>System.Exception</td>
       <td>System.Object</td>
       <td></td>
     </tr>
     <tr>
       <td>java.lang.RuntimeException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception</td>
       <td>System.Exception</td>
       <td>System.Object</td>
       <td></td>
     </tr>
     <tr>
       <td>java.lang.ClassNotFoundException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception <br/>--- java.lang.ReflectiveOperationException</td>
       <td>None</td>
       <td></td>
       <td>Reflection in .NET has no equivalent. However, in places where it is used we are not wrapping any exceptions in an Exception instance, which may mean there could be problems with excpetions getting caught in calling code and not handled appropriately.</td>
     </tr>
     <tr>
       <td>java.util.NoSuchElementException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception <br/>--- java.lang.RuntimeException</td>
       <td>None</td>
       <td></td>
       <td>.NET enumerators do not throw when there are no more elements, they return false. Some places are catching InvalidOperationException instead, but they shouldn't be.</td>
     </tr>
     <tr>
       <td>org.xml.sax.SAXException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception</td>
       <td>Sax.SaxException</td>
       <td>System.Object <br/>- System.Exception</td>
       <td>Exists only in the Support folder of Lucene.Net.Benchmark.</td>
     </tr>
     <tr>
       <td>java.lang.InterruptedException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception</td>
       <td>System.Threading.ThreadInterruptedException</td>
       <td>System.Object <br/>- System.Exception <br/>-- System.SystemException</td>
       <td></td>
     </tr>
     <tr>
       <td>org.apache.lucene.util.ThreadInterruptedException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception <br/>-- System.SystemException</td>
       <td>System.Threading.ThreadInterruptedException</td>
       <td>System.Object <br/>- System.Exception <br/>-- System.SystemException</td>
       <td>This class exists to convert the exception into RuntimeException. It is possible we have missed something fundamental here.</td>
     </tr>
     <tr>
       <td>org.apache.lucene.benchmark.bytask.feeds.NoMoreDataException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception</td>
       <td>Lucene.Net.Benchmark.ByTask.Feeds.NoMoreDataException</td>
       <td>System.Object <br/>- System.Exception</td>
       <td></td>
     </tr>
     <tr>
       <td>java.lang.NumberFormatException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception <br/>--- java.lang.RuntimeException <br/>---- java.lang.IllegalArgumentException</td>
       <td>System.FormatException</td>
       <td>System.Object <br/>- System.Exception <br/>-- System.SystemException</td>
       <td>It is possible we missed some catch blocks because FormatExcption doesn't subclass ArgumentException, needs review to make sure the correct exception types are being thrown.</td>
     </tr>
     <tr>
       <td>org.apache.commons.compress.compressors.CompressorException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception</td>
       <td>None</td>
       <td></td>
       <td>.NET and SharpZipLib do not throw when instantiating a compression stream.</td>
     </tr>
     <tr>
       <td>java.util.zip.DataFormatException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception</td>
       <td>None</td>
       <td></td>
       <td>We aren't catching an exception for compression streams, but this needs review.</td>
     </tr>
     <tr>
       <td>java.lang.SecurityException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception <br/>-- java.lang.RuntimeException</td>
       <td>System.Security.SecurityException</td>
       <td>System.Object <br/>- System.Exception <br/>-- System.SystemException</td>
       <td></td>
     </tr>
     <tr>
       <td rowspan="2">java.nio.file.NoSuchFileException</td>
       <td rowspan="2">java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception <br/>-- java.io.IOException <br/>-- java.nio.file.FileSystemException</td>
       <td>System.IO.FileNotFoundException</td>
       <td>System.Object <br/>- System.Exception <br/>-- System.SystemException <br/>--- System.IO.IOException</td>
       <td></td>
     </tr>
     <tr>
       <td>System.IO.DirectoryNotFoundException</td>
       <td>System.Object <br/>- System.Exception <br/>-- System.SystemException <br/>--- System.IO.IOException</td>
       <td></td>
     </tr>
     <tr>
       <td>org.apache.lucene.store.NoSuchDirectoryException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception <br/>--- java.io.IOException <br/>---- java.io.FileNotFoundDirectory</td>
       <td>System.IO.DirectoryNotFoundException</td>
       <td>System.Object <br/>- System.Exception <br/>-- System.SystemException <br/>--- System.IO.IOException</td>
       <td></td>
     </tr>
     <tr>
       <td>java.lang.OutOfMemoryError</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Error <br/>--- java.lang.VirtualMachineError</td>
       <td>System.OutOfMemoryException</td>
       <td>System.Object <br/>- System.Exception <br/>-- System.SystemException</td>
       <td>Microsoft recommends calling Environment.FailFast() in this case, but we need a review to ensure we are doing so. I believe some tests are also marked [Ignore] because it was assumed that OutOfMemoryException cannot be caught in .NET.</td>
     </tr>
     <tr>
       <td>org.apache.lucene.store.AlreadyClosedException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception <br/>--- java.lang.RuntimeException <br/>--- java.lang.IllegalStateException</td>
       <td>System.ObjectDisposedException</td>
       <td>System.Object <br/>- System.Exception <br/>-- System.SystemException <br/>-- System.InvalidOperationException</td>
       <td>In prior versions of Lucene.NET, there were at least attempts to use both ICloseable and IDisposable. We need to revisit this.</td>
     </tr>
     <tr>
       <td>java.nio.charset.CharacterCodingException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception <br/>--- java.io.IOException</td>
       <td>All</td>
       <td></td>
       <td>Perhaps catching all exceptions is not the right decision here.</td>
     </tr>
     <tr>
       <td>org.apache.lucene.util.BytesRefHash.MaxBytesLengthExceededException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception <br/>--- java.lang.RuntimeException</td>
       <td>Lucene.Net.Util.BytesRefHash.MaxBytesLengthExceededException</td>
       <td>System.Object <br/>- System.Exception</td>
       <td></td>
     </tr>
     <tr>
       <td>org.apache.lucene.search.CollectionTerminatedException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception <br/>--- java.lang.RuntimeException</td>
       <td>Lucene.Net.Search.CollectionTerminatedException</td>
       <td>System.Object <br/>- System.Exception</td>
       <td></td>
     </tr>
     <tr>
       <td>java.util.concurrent.ExecutionException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception</td>
       <td>All</td>
       <td></td>
       <td>Needs review.</td>
     </tr>
     <tr>
       <td>java.nio.BufferUnderflowException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception <br/>--- java.lang.RuntimeException</td>
       <td>J2N.IO.BufferUnderflowException</td>
       <td>System.Object <br/>- System.Exception</td>
       <td></td>
     </tr>
     <tr>
       <td>java.lang.ClassCastException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception <br/>--- java.lang.RuntimeException</td>
       <td>J2N.InvalidCastException</td>
       <td>System.Object <br/>- System.Exception <br/>-- System.SystemException</td>
       <td></td>
     </tr>
     <tr>
       <td>org.apache.lucene.store.LockObtainFailedException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception <br/>--- java.io.IOException</td>
       <td>Lucene.Net.Store.LockObtainFailedException</td>
       <td>System.Object <br/>- System.Exception <br/>-- System.SystemException <br/>--- System.IO.IOException</td>
       <td></td>
     </tr>
     <tr>
       <td>java.security.PrivilegedActionException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception</td>
       <td>None</td>
       <td></td>
       <td>This exception is used to work around a bug in Java in MMapDirectory.</td>
     </tr>
     <tr>
       <td>java.nio.channels.OverlappingFileLockException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception <br/>--- java.lang.RuntimeException <br/>---- java.lang.IllegalStateException</td>
       <td>None</td>
       <td></td>
       <td>NativeFSLockFactory was redesigned for .NET and this exception was factored out.</td>
     </tr>
     <tr>
       <td rowspan="2">java.lang.AssertionError</td>
       <td rowspan="2">java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Error</td>
       <td>System.InvalidOperationException</td>
       <td>System.Object <br/>- System.Exception <br/>-- System.SystemException</td>
       <td>Lucene.Net.Diagnostics.AssertionException was added later. Should we migrate everything over to it?</td>
     </tr>
     <tr>
       <td>Lucene.Net.Diagnostics.AssertionException</td>
       <td>System.Object <br/>- System.Exception</td>
       <td>This is only used in Debugging.Assert() method calls.</td>
     </tr>
     <tr>
       <td>java.lang.Error</td>
       <td>java.lang.Object <br/>- java.lang.Throwable</td>
       <td>System.Exception</td>
       <td>System.Object</td>
       <td></td>
     </tr>
     <tr>
       <td>java.io.EOFException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception <br/>-- java.io.IOException</td>
       <td>System.IO.EndOfStreamException</td>
       <td>System.Object <br/>- System.Exception <br/>-- System.SystemException <br/>--- System.IO.IOException</td>
       <td></td>
     </tr>
     <tr>
       <td>java.lang.IllegalStateException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception <br/>--- java.lang.RuntimeException</td>
       <td>System.InvalidOperationException</td>
       <td>System.Object <br/>- System.Exception <br/>-- System.SystemException</td>
       <td></td>
     </tr>
     <tr>
       <td>org.apache.lucene.index.IndexFormatTooOldException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception <br/>--- java.io.IOException <br/>--- org.apache.lucene.index.CorruptIndexException</td>
       <td>Lucene.Net.Index.IndexFormatTooOldException</td>
       <td>System.Object <br/>- System.Exception <br/>-- System.SystemException <br/>-- System.IO.IOException</td>
       <td></td>
     </tr>
     <tr>
       <td>org.apache.lucene.index.CorruptIndexException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception <br/>--- java.io.IOException <br/>--- org.apache.lucene.index.CorruptIndexException</td>
       <td>Lucene.Net.Index.IndexFormatTooOldException</td>
       <td>System.Object <br/>- System.Exception <br/>-- System.SystemException <br/>-- System.IO.IOException</td>
       <td></td>
     </tr>
     <tr>
       <td>org.apache.lucene.index.TestCrashCausesCorruptIndex.CrashingException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception <br/>--- java.lang.RuntimeException</td>
       <td>Lucene.Net.Index.TestCrashCausesCorruptIndex.CrashingException</td>
       <td>System.Object <br/>- System.Exception</td>
       <td>Only exists in one test class.</td>
     </tr>
     <tr>
       <td>org.apache.lucene.util.SetOnce.AlreadySetException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception <br/>--- java.lang.RuntimeException <br/>---- java.lang.IllegalStateException</td>
       <td>Lucene.Net.Util.AlreadySetException</td>
       <td>System.Object <br/>- System.Exception <br/>-- System.SystemException <br/>-- System.InvalidOperationException</td>
       <td></td>
     </tr>
     <tr>
       <td>org.apache.lucene.store.MockDirectoryWrapper.FakeIOException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception <br/>--- java.io.IOException</td>
       <td>Lucene.Net.Store.FakeIOException</td>
       <td>System.Object <br/>- System.Exception <br/>-- System.SystemException <br/>-- System.IO.IOException</td>
       <td></td>
     </tr>
     <tr>
       <td>org.apache.lucene.search.TimeLimitingCollector.TimeExceededException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception <br/>--- java.lang.RuntimeException</td>
       <td>Lucene.Net.Search.TimeLimitingCollector.TimeExceededException</td>
       <td>System.Object <br/>- System.Exception</td>
       <td></td>
     </tr>
     <tr>
       <td>org.antlr.runtime.RecognitionException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception</td>
       <td>Antlr.Runtime.RecognitionException</td>
       <td>System.Object <br/>- System.Exception</td>
       <td></td>
     </tr>
     <tr>
       <td>java.lang.StackOverflowError</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Error <br/>--- java.lang.VirtualMachineError</td>
       <td>System.StackOverflowException</td>
       <td>System.Object <br/>- System.Exception <br/>-- System.SystemException</td>
       <td>Uncatchable in .NET. Microsoft recommends breaking out of recursive calls with a max value setting.</td>
     </tr>
     <tr>
       <td>java.lang.ArithmeticException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception <br/>--- java.lang.RuntimeException</td>
       <td>System.ArithmeticException</td>
       <td>System.Object <br/>- System.Exception <br/>-- System.SystemException</td>
       <td></td>
     </tr>
     <tr>
       <td>org.apache.lucene.search.BooleanQuery.TooManyClauses</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception <br/>--- java.lang.RuntimeException</td>
       <td>Lucene.Net.Search.BooleanQuery.TooManyClausesException</td>
       <td>System.Object <br/>- System.Exception</td>
       <td></td>
     </tr>
     <tr>
       <td>org.apache.lucene.queryparser.classic.ParseException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception</td>
       <td>Lucene.Net.QueryParsers.Classic.ParseException</td>
       <td>System.Object <br/>- System.Exception</td>
       <td></td>
     </tr>
     <tr>
       <td>org.apache.lucene.queryparser.classic.TokenMgrError</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Error</td>
       <td>Lucene.Net.QueryParsers.Classic.TokenMgrError</td>
       <td>System.Object <br/>- System.Exception</td>
       <td></td>
     </tr>
     <tr>
       <td>java.util.MissingResourceException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception <br/>--- java.lang.RuntimeException</td>
       <td>System.Resources.MissingManifestResourceException </td>
       <td>System.Object <br/>- System.Exception <br/>-- System.SystemException</td>
       <td></td>
     </tr>
     <tr>
       <td>org.apache.lucene.queryparser.flexible.standard.parser.ParseException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception <br/>--- org.apache.lucene.queryparser.flexible.core.QueryNodeException <br/>---- org.apache.lucene.queryparser.flexible.core.QueryNodeParseException</td>
       <td>Lucene.Net.QueryParsers.Flexible.Standard.Parser.ParseException</td>
       <td>System.Object <br/>- System.Exception <br/>-- Lucene.Net.QueryParsers.Flexible.Core.QueryNodeException <br/>--- Lucene.Net.QueryParsers.Flexible.Core.QueryNodeParseException</td>
       <td></td>
     </tr>
     <tr>
       <td>org.apache.lucene.queryparser.flexible.standard.parser.ParseException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception</td>
       <td>Lucene.Net.QueryParsers.Flexible.Standard.Parser.ParseException</td>
       <td>System.Object <br/>- System.Exception <br/>-- Lucene.Net.QueryParsers.Flexible.Core.QueryNodeException</td>
       <td></td>
     </tr>
     <tr>
       <td>org.apache.lucene.queryparser.flexible.standard.parser.TokenMgrError</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Error</td>
       <td>Lucene.Net.QueryParsers.Flexible.Standard.Parser.TokenMgrError</td>
       <td>System.Object <br/>- System.Exception</td>
       <td></td>
     </tr>
     <tr>
       <td>org.apache.lucene.queryparser.surround.parser.TokenMgrError</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Error</td>
       <td>Lucene.Net.QueryParsers.Surround.Parser.TokenMgrError</td>
       <td>System.Object <br/>- System.Exception</td>
       <td></td>
     </tr>
     <tr>
       <td>org.apache.lucene.queryparser.surround.ParseException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception</td>
       <td>Lucene.Net.QueryParsers.Surround.ParseException</td>
       <td>System.Object <br/>- System.Exception</td>
       <td></td>
     </tr>
     <tr>
       <td>org.apache.lucene.queryparser.xml.ParserException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception</td>
       <td>Lucene.Net.QueryParsers.Xml.ParserException</td>
       <td>System.Object <br/>- System.Exception</td>
       <td></td>
     </tr>
     <tr>
       <td>org.apache.lucene.replicator.SessionExpiredException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception <br/>--- java.io.IOException</td>
       <td>Lucene.Net.Replicator.SessionExpiredException</td>
       <td>System.Object <br/>- System.Exception <br/>-- System.SystemException <br/>-- System.IO.IOException</td>
       <td></td>
     </tr>
     <tr>
       <td>java.lang.NoClassDefFoundError</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Error <br/>--- java.lang.LinkageError</td>
       <td>System.TypeLoadException</td>
       <td>System.Object <br/>- System.Exception <br/>-- System.SystemException</td>
       <td>Review is required to determine if this is the correct exception in .NET.</td>
     </tr>
     <tr>
       <td>org.junit.AssumptionViolatedException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception <br/>--- java.lang.RuntimeException <br/>---- org.junit.internal.AssumptionViolatedException</td>
       <td>NUnit.Framework.InconclusiveException</td>
       <td>System.Object <br/>- System.Exception <br/>-- NUnit.Framework.ResultStateException</td>
       <td></td>
     </tr>
     <tr>
       <td>java.nio.file.AccessDeniedException</td>
       <td>java.lang.Object <br/>- java.lang.Throwable <br/>-- java.lang.Exception <br/>--- java.io.IOException <br/>---- java.nio.file.FileSystemException</td>
       <td>System.UnauthorizedAccessException </td>
       <td>System.Object <br/>- System.Exception <br/>-- System.SystemException</td>
       <td>Special case: Does not subclass IOException in .NET but does in Java.</td>
     </tr>
   </table>
   
   
   </details>
   
   There is a lot of info in the above table, so let me just pick out a few cases to demonstrate the kind of issues we are facing.
   
   | Exception Type  | Issue  | 
   |---|---|
   | `java.text.ParseException`  | This excpetion was translated to `FormatException` in some places, `InvalidOperationException` in some places, and `Excpetion` in other places (there may be more). | 
   | `java.nio.file.AccessDeniedException`  | The corresponding class in .NET, `System.UnauthorizedAccessException` does not subclass `IOException`. As a result, catch blocks for `IOException` are missing this one when they should be catching it.  |
   | `java.io.FileNotFoundException` and `java.nio.file.NoSuchFileException`  | In .NET there is only 1 exception type for dealing with missing files. However, in Java these exceptions are also thrown if there is a missing directory.  | 
   | `org.apache.lucene.store.NoSuchDirectoryException`  | This class was excluded from Lucene.NET because .NET already has a `System.IO.DirectoryNotFoundException`, however it does not subclass `System.IO.FileNotFoundException`, so we needed to manually duplicate `catch` blocks everywhere `System.IO.FileNotFoundExeption` was caught and duplicate error handler logic. | 
   | `java.lang.OutOfMemoryError`  | This excpetion derives from `java.lang.Error`. This means in .NET when we catch `Exception`, we are catching this exception when we shouldn't be in many cases and not allowing it to propagate to its intended handler.  | 
   | `java.lang.AssertionError` | Since in .NET assertions are compiled out of the release and the behavior of `Debug.Assert()` doesn't consistently throw an exception that can be caught, an equivalent for this exception wasn't added until recently and is not in use in every place it was in Lucene. However, it is also meant to be excluded from exception handling and propagate to the top of the stack, but wouldn't be if we were using it for the same reason as `java.lang.OutOfMemoryError`. | 
   | `java.lang.NullPointerException` | Since in Java the approach is to let exceptions fly and handle them later, this exception is being caught in Lucene. However, in most cases when there is a `null` parameter in .NET, the expected exception is `ArgumentNullException` rather than `NullReferenceException`.  `NullReferenceException` is an unexpected exception in .NET we would expect to let propagate so we can prevent it from being thrown more easily. `NullReferenceException` was used in a lot of catch blocks, but this may be hiding problems in the code where exceptions can be avoided.  | 
   
   ## Challenges
   
   - Fill gaps between Java and .NET exception types in a way that doesn't rely 100% on inheritance, since we have several special cases to deal with in mulitple places in the application
   - Avoid exception translation problems when porting from Java to .NET
   - Make it possible to maintain this easily - if we discover a rule about Java exceptions we missed, it should be possible to add that rule in only 1 place
   - Make the code mostly or completely self-documenting so we don't always have to refer to documentation in order to translate exception types right
   
   ## Proposed Solution for Catching Exceptions
   
   We can take advantage of the C# [`when` in a `catch` statement](https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/when#when-in-a-catch-statement) to make complex rules in catch blocks. In addition, we can use extension methods on `Exception` with names that match exception groups in Java to apply the rules specifc to that group.
   
   ### Example
   
   ```c#
   public static class ExceptionExtensions
   {
       public static bool IsThrowable(this Exception e)
       {
           return true; // All errors inherit from Throwable in Java
       }
   
       public static bool IsError(this Exception e)
       {
           return e is OutOfMemoryException ||
               e is AssertionException ||
               e is StackOverflowException;
       }
   
       public static bool IsException(this Exception e)
       {
           return e is Exception && !IsError(e);
       }
   
       public static bool IsIOException(this Exception e)
       {
           return e is IOException || e is UnauthorizedAccessException;
       }
   
       public static bool IsFileNotFoundOrNoSuchFileExcetption(this Exception e)
       {
           return e is FileNotFoundException || e is DirectoryNotFoundException;
       }
   }
   ```
   
   #### FileNotFoundException Usage
   
   <details>
     <summary>Click to expand</summary>
   
   ##### Java
   
   ```java
   public static boolean slowFileExists(Directory dir, String fileName) throws IOException {
       try {
           dir.openInput(fileName, IOContext.DEFAULT).close();
           return true;
       } catch (NoSuchFileException | FileNotFoundException e) {
           return false;
       }
   }
   ```
   
   ##### C# Before
   
   ```c#
   public static bool SlowFileExists(Directory dir, string fileName)
   {
       try
       {
           dir.OpenInput(fileName, IOContext.DEFAULT).Dispose();
           return true;
       }
       catch (FileNotFoundException)
       {
           return false;
       }
       // LUCENENET specific - .NET (thankfully) only has one FileNotFoundException, so we don't need this
       //catch (NoSuchFileException)
       //{
       //    return false;
       //}
       // LUCENENET specific - since NoSuchDirectoryException subclasses FileNotFoundException
       // in Lucene, we need to catch it here to be on the safe side.
       catch (DirectoryNotFoundException)
       {
           return false;
       }
   }
   ```
   
   ##### C# After
   
   ```c#
   public static bool SlowFileExists(Directory dir, string fileName)
   {
       try
       {
           dir.OpenInput(fileName, IOContext.DEFAULT).Dispose();
           return true;
       }
       catch (Exception e) when (e.IsFileNotFoundOrNoSuchFileExcetption())
       {
           return false;
       }
   }
   ```
   </details>
   
   #### IOException Usage
   
   <details>
     <summary>Click to expand</summary>
   
   ##### Java
   
   ```java
   try {
     TestUtil.rm(everything);
   } catch (IOException e) {
     Class<?> suiteClass = RandomizedContext.current().getTargetClass();
     if (suiteClass.isAnnotationPresent(SuppressTempFileChecks.class)) {
       System.err.println("WARNING: Leftover undeleted temporary files (bugUrl: "
           + suiteClass.getAnnotation(SuppressTempFileChecks.class).bugUrl() + "): "
           + e.getMessage());
       return;
     }
     throw e;
   }
   ```
   
   ##### C# Before
   
   ```c#
   try
   {
       TestUtil.Rm(everything);
   }
   // LUCENENET specific: UnauthorizedAccessException doesn't subclass IOException as
   // AccessDeniedException does in Java, so we need a special case for it.
   catch (UnauthorizedAccessException e)
   {
       //                    Type suiteClass = RandomizedContext.Current.GetTargetType;
       //                    if (suiteClass.IsAnnotationPresent(typeof(SuppressTempFileChecks)))
       //                    {
       Console.Error.WriteLine("WARNING: Leftover undeleted temporary files " + e.Message);
       return;
       //                    }
   }
   catch (IOException e)
   {
       //                    Type suiteClass = RandomizedContext.Current.GetTargetType;
       //                    if (suiteClass.IsAnnotationPresent(typeof(SuppressTempFileChecks)))
       //                    {
       Console.Error.WriteLine("WARNING: Leftover undeleted temporary files " + e.Message);
       return;
       //                    }
   }
   ```
   
   ##### C# After
   
   ```c#
   try
   {
       TestUtil.Rm(everything);
   }
   catch (Exception e) when (e.IsIOException())
   {
       //                    Type suiteClass = RandomizedContext.Current.GetTargetType;
       //                    if (suiteClass.IsAnnotationPresent(typeof(SuppressTempFileChecks)))
       //                    {
       Console.Error.WriteLine("WARNING: Leftover undeleted temporary files " + e.Message);
       return;
       //                    }
   }
   ```
   
   </details>
   
   > **NOTE:** There are dozens of places where we catch `IOExcpetion` that don't take `UnauthorizedAccessException` into account.
   >
   > Also, there may be other `IOExcption`-derived types in Java that don't derive from `IOExcpetion` in .NET. We need to review all of the [direct known subclasses](https://docs.oracle.com/javase/7/docs/api/java/io/IOException.html) to be relatively sure we are catching everything that is supposed to be caught.
   
   ## Proposed Solution for Throwing Exceptions
   
   Subclass .NET exceptions with internal classes that use Java exception names. This takes the guesswork out of deciding which excpetion is the appropriate one to throw in most cases, and we still throw the correct .NET exception types (that can be caught) to the outside world.
   
   There are several exceptions in Java where in .NET we don't get an exception, but may get a `false` or `null` return value instead. We can create fake excpetions for these and mark them with the `[Obsolete]` attribute with a message indicating how to convert the .NET code.
   
   ### Example
   
   ```c#
   internal class ParseException : FormatException
   {
       private const string ERROR_OFFSET_NAME = "__errorOffset";
   
       public ParseException() {}
   
       public ParseException(string message) : base(message) {}
   
       public ParseException(string message, Exception innerException) : base(message, innerException) {}
   
       public int ErrorOffset
       {
           get => Convert.ToInt32(Data[ERROR_OFFSET_NAME] ?? 0); // Data is passed on in FormatException so it is accessible even though this class is internal
           set => Data[ERROR_OFFSET_NAME] = value;
       }
   
       public override Message => base.Message + ", line: " + ErrorOffset;
   }
   
   [Obsolete("This exception is thrown by Reflection code in Java when calling private/internal members of a Type, but is unnecessary in .NET.")]
   internal class PrivilegedActionException : Exception
   {
   }
   
   [Obsolete(".NET enumerators do not throw when there are no more elements. It is recommended to convert iterators into enumerators and use a MoveNext() method that returns false.")]
   internal class NoSuchElementException: Exception
   {
   }
   
   [Obsolete("Reflection doesn't throw exceptions when it cannot locate a type in .NET, instead it will return null. Do not catch this exception and adjust the logic to consider a null return value the equivalent of the error condition.")]
   internal class ClassNotFoundException : Exception
   {
   }
   
   ```
   
   #### ParseException Usage
   
   <details>
     <summary>Click to expand</summary>
   
   ##### Java
   
   ```java
   
   if (line.startsWith(KEEPCASE_KEY)) {
     String parts[] = line.split("\\s+");
     if (parts.length != 2) {
       throw new ParseException("Illegal KEEPCASE declaration", reader.getLineNumber());
     }
     keepcase = flagParsingStrategy.parseFlag(parts[1]);
   }
   
   ```
   
   ##### C# Before
   
   ```c#
   if (line.StartsWith(KEEPCASE_KEY, StringComparison.Ordinal))
   {
       string[] parts = whitespacePattern.Split(line).TrimEnd();
       if (parts.Length != 2)
       {
           throw new FormatException(string.Format("Illegal KEEPCASE declaration, line {0}", lineNumber));
       }
       keepcase = flagParsingStrategy.ParseFlag(parts[1]);
   }
   ```
   
   ##### C# After
   
   ```c#
   if (line.StartsWith(KEEPCASE_KEY, StringComparison.Ordinal))
   {
       string[] parts = whitespacePattern.Split(line).TrimEnd();
       if (parts.Length != 2)
       {
           throw new ParseException("Illegal KEEPCASE declaration") { ErrorOffset = lineNumber };
       }
       keepcase = flagParsingStrategy.ParseFlag(parts[1]);
   }
   ```
   
   </details>


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



Mime
View raw message