-
Notifications
You must be signed in to change notification settings - Fork 639
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Review exception types thrown and in try/catch blocks #446
Labels
Milestone
Comments
NightOwl888
added
Project Infrastructure
design
is:enhancement
New feature or request
is:idea
pri:high
labels
Mar 20, 2021
11 tasks
NightOwl888
added a commit
to NightOwl888/lucenenet
that referenced
this issue
Apr 14, 2021
…ng Java exception types to their closest .NET equivalent. See apache#446.
NightOwl888
added a commit
to NightOwl888/lucenenet
that referenced
this issue
Apr 14, 2021
…erted them to use our IsThrowable() extension method (see apache#446).
NightOwl888
added a commit
to NightOwl888/lucenenet
that referenced
this issue
Apr 14, 2021
…d them to use our IsError() extension method (see apache#446).
NightOwl888
added a commit
to NightOwl888/lucenenet
that referenced
this issue
Apr 14, 2021
…erted them to use our IsException() extension method (see apache#446). Added overload for printStackTrace() that accepts a TextWriter so we can specify another output.
NightOwl888
added a commit
to NightOwl888/lucenenet
that referenced
this issue
Apr 14, 2021
…nverted them to use our IsIOException() extension method (see apache#446).
NightOwl888
added a commit
to NightOwl888/lucenenet
that referenced
this issue
Apr 14, 2021
…eption and converted them to use our IsIllegalArgumentException() extension method (see apache#446).
NightOwl888
added a commit
to NightOwl888/lucenenet
that referenced
this issue
Apr 14, 2021
…gException and converted them to use our IsUnsupportedEncodingException() extension method (see apache#446).
NightOwl888
added a commit
to NightOwl888/lucenenet
that referenced
this issue
Apr 14, 2021
…ion and converted them to use our IsIllegalStateException() extension method (see apache#446).
NightOwl888
added a commit
to NightOwl888/lucenenet
that referenced
this issue
Apr 14, 2021
… converted them to use our IsParseException() extension method (see apache#446).
NightOwl888
added a commit
to NightOwl888/lucenenet
that referenced
this issue
Apr 14, 2021
…ion and converted them to use our IsNumberFormatException() extension method (see apache#446).
NightOwl888
added a commit
to NightOwl888/lucenenet
that referenced
this issue
Apr 14, 2021
…n || FileNotFoundException and converted them to use our IsNoSuchFileExceptionOrFileNotFoundException() extension method (see apache#446).
NightOwl888
added a commit
to NightOwl888/lucenenet
that referenced
this issue
Apr 14, 2021
…onException and converted them to use our IsUnsupportedOperationException() extension method (see apache#446).
NightOwl888
added a commit
to NightOwl888/lucenenet
that referenced
this issue
Apr 14, 2021
…onverted them to use our IsEOFException() extension method (see apache#446).
NightOwl888
added a commit
to NightOwl888/lucenenet
that referenced
this issue
Apr 14, 2021
…eption and converted them to use our IsMissingResourceException() extension method (see apache#446).
NightOwl888
added a commit
to NightOwl888/lucenenet
that referenced
this issue
Apr 14, 2021
…or and converted them to use our IsNoClassDefFoundError() extension method (see apache#446). Also, changed some catch blocks that were using TypeLoadException to use IsClassNotFoundException(), as there are places where they overlap.
NightOwl888
added a commit
to NightOwl888/lucenenet
that referenced
this issue
Apr 14, 2021
…n and converted them to use our IsArithmeticException() extension method (see apache#446).
NightOwl888
added a commit
to NightOwl888/lucenenet
that referenced
this issue
Apr 14, 2021
…laced the exception that is thrown in Lucene.NET with Lucene.Net.Diagnostics.AssertionException (see apache#446).
NightOwl888
added a commit
to NightOwl888/lucenenet
that referenced
this issue
Apr 14, 2021
… converted them to use our IsAssertionError() extension method (see apache#446).
NightOwl888
added a commit
to NightOwl888/lucenenet
that referenced
this issue
Apr 14, 2021
…eption and converted them to use our IsIndexOutOfBoundsException() extension method (see apache#446).
NightOwl888
added a commit
that referenced
this issue
Apr 26, 2021
…nvalidOperationException instead of Exception (see #446).
NightOwl888
added a commit
that referenced
this issue
Apr 26, 2021
…arseException.Create() (see #446).
NightOwl888
added a commit
that referenced
this issue
Apr 26, 2021
NightOwl888
added a commit
that referenced
this issue
Apr 26, 2021
…ements to ServiceConfigurationError.Create() (see #446)
NightOwl888
added a commit
that referenced
this issue
Apr 26, 2021
…Exception when the set is readonly, not InvalidOperationException to match .NET collection behavior (see #446).
NightOwl888
added a commit
that referenced
this issue
Apr 26, 2021
…umer(): Throw NotSupportedException rather than InvalidOperationException (see #446).
NightOwl888
added a commit
that referenced
this issue
Apr 26, 2021
…c(): Throw AssertionError rather than InvalidOperationException (see #446).
NightOwl888
added a commit
that referenced
this issue
Apr 26, 2021
…ertionError rather than InvalidOperationException (see #446).
NightOwl888
added a commit
that referenced
this issue
Apr 26, 2021
…nError rather than InvalidOperationException (see #446).
NightOwl888
added a commit
that referenced
this issue
Apr 26, 2021
…ements to IllegalStateException.Create() (see #446).
NightOwl888
added a commit
that referenced
this issue
Apr 26, 2021
…ts to UnsupportedOperationException.Create() (see #446)
NightOwl888
added a commit
that referenced
this issue
Apr 26, 2021
…ception rather than InvalidOperationException (see #446)
NightOwl888
added a commit
that referenced
this issue
Apr 26, 2021
…ception rather than InvalidOperationException (see #446)
NightOwl888
added a commit
that referenced
this issue
Apr 26, 2021
…nto ClassNotFoundException.Create() (see #446)
NightOwl888
added a commit
that referenced
this issue
Apr 26, 2021
…constructors Obsolete with instructions how to convert (see #446).
NightOwl888
added a commit
that referenced
this issue
Apr 26, 2021
…cumentation to explain how to convert (see #446).
NightOwl888
added a commit
that referenced
this issue
Apr 26, 2021
…d documentation to explain how to convert (see #446).
NightOwl888
added a commit
that referenced
this issue
Apr 26, 2021
…d StringIndexOutOfBoundsException with static factory methods and added documentation to explain how to convert (see #446)
NightOwl888
added a commit
that referenced
this issue
Apr 26, 2021
…ents into AlreadyClosedException.Create() (see #446)
NightOwl888
added a commit
that referenced
this issue
Apr 26, 2021
…o NumberFormatException.Create() (see #446)
NightOwl888
added a commit
that referenced
this issue
Apr 26, 2021
…s into OutOfMemoryError.Create() (see #446)
NightOwl888
added a commit
that referenced
this issue
Apr 26, 2021
…ption whereever Lucene thorws ParseException (see #446)
NightOwl888
added a commit
that referenced
this issue
Apr 26, 2021
… correspond to RuntimeException in Java (see #446).
NightOwl888
added a commit
that referenced
this issue
Apr 26, 2021
…processors back to using FormatException, because that is the expected parse error in .NET (see #446)
NightOwl888
added a commit
that referenced
this issue
Apr 26, 2021
…ow ArgumentNullException (see #446)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 ofIndexWriter
,IndexReader
and many types that support them. Removing the behavior 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 fromjava.lang.RuntimeException
. As a result, a common thing to do throughout the Lucene codebase is to catchjava.lang.Exception
(and all derived classes of it) and wrap it in ajava.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.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:
System.Exception
to replacejava.lang.Throwable
,java.lang.Error
,java.lang.Exception
, andjava.lang.RuntimeException
without considering all of the ramifications.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.
Click for details
- java.lang.Throwable
-- java.lang.Exception
- System.Exception
-- System.SystemException
- java.lang.Throwable
-- java.lang.Exception
--- java.lang.RuntimeException
-- java.lang.IndexOutOfBoundsException
- System.Exception
-- System.SystemException
- System.Exception
-- System.SystemException
--- System.ArgumentException
- java.lang.Throwable
-- java.lang.Exception
--- java.lang.RuntimeException
-- java.lang.IndexOutOfBoundsException
- System.Exception
-- System.SystemException
- System.Exception
-- System.SystemException
--- System.ArgumentException
- java.lang.Throwable
-- java.lang.Exception
--- java.lang.RuntimeException
- System.Exception
-- System.SystemException
- System.Exception
-- System.SystemException
--- System.ArgumentException
- java.lang.Throwable
-- java.lang.Exception
- System.Exception
-- System.SystemException
- System.Exception
-- System.SystemException
- java.lang.Throwable
- java.lang.Throwable
-- java.lang.Exception
- java.lang.Throwable
-- java.lang.Exception
--- java.io.IOException
- System.Exception
-- System.SystemException
--- System.IO.IOException
- System.Exception
-- System.SystemException
--- System.IO.IOException
- java.lang.Throwable
-- java.lang.Exception
--- java.lang.ReflectiveOperationException
- System.Exception
-- System.SystemException
--- System.IO.IOException
- java.lang.Throwable
-- java.lang.Exception
--- java.lang.ReflectiveOperationException
- System.Exception
-- System.ApplicationException
- java.lang.Throwable
-- java.lang.Exception
--- java.lang.ReflectiveOperationException
- System.Exception
- System.Exception
-- System.SystemException<
- java.lang.Throwable
-- java.lang.Exception
--- java.lang.RuntimeException
- System.Exception
-- System.SystemException
- System.Exception
-- System.SystemException
--- System.ArgumentException
- System.Exception
-- System.SystemException
--- System.ArgumentException
- java.lang.Throwable
-- java.lang.Exception
--- java.lang.RuntimeException
- System.Exception
-- System.SystemException
- java.lang.Throwable
-- java.lang.Exception
--- java.lang.ReflectiveOperationException
- System.Exception
-- System.SystemException
- java.lang.Throwable
-- java.lang.Exception
--- java.lang.RuntimeException
- System.Exception
-- System.SystemException
- java.lang.Throwable
-- java.lang.Exception
- java.lang.Throwable
-- java.lang.Exception
--- java.lang.ReflectiveOperationException
- java.lang.Throwable
-- java.lang.Exception
--- java.lang.RuntimeException
- java.lang.Throwable
-- java.lang.Exception
- System.Exception
- java.lang.Throwable
-- java.lang.Exception
- System.Exception
-- System.SystemException
- java.lang.Throwable
-- java.lang.Exception
-- System.SystemException
- System.Exception
-- System.SystemException
- java.lang.Throwable
-- java.lang.Exception
- System.Exception
- java.lang.Throwable
-- java.lang.Exception
--- java.lang.RuntimeException
---- java.lang.IllegalArgumentException
- System.Exception
-- System.SystemException
- java.lang.Throwable
-- java.lang.Exception
- java.lang.Throwable
-- java.lang.Exception
- java.lang.Throwable
-- java.lang.Exception
-- java.lang.RuntimeException
- System.Exception
-- System.SystemException
- java.lang.Throwable
-- java.lang.Exception
-- java.io.IOException
-- java.nio.file.FileSystemException
- System.Exception
-- System.SystemException
--- System.IO.IOException
- System.Exception
-- System.SystemException
--- System.IO.IOException
- java.lang.Throwable
-- java.lang.Exception
--- java.io.IOException
---- java.io.FileNotFoundDirectory
- System.Exception
-- System.SystemException
--- System.IO.IOException
- java.lang.Throwable
-- java.lang.Error
--- java.lang.VirtualMachineError
- System.Exception
-- System.SystemException
- java.lang.Throwable
-- java.lang.Exception
--- java.lang.RuntimeException
--- java.lang.IllegalStateException
- System.Exception
-- System.SystemException
-- System.InvalidOperationException
- java.lang.Throwable
-- java.lang.Exception
--- java.io.IOException
- java.lang.Throwable
-- java.lang.Exception
--- java.lang.RuntimeException
- System.Exception
- java.lang.Throwable
-- java.lang.Exception
--- java.lang.RuntimeException
- System.Exception
- java.lang.Throwable
-- java.lang.Exception
- java.lang.Throwable
-- java.lang.Exception
--- java.lang.RuntimeException
- System.Exception
- java.lang.Throwable
-- java.lang.Exception
--- java.lang.RuntimeException
- System.Exception
-- System.SystemException
- java.lang.Throwable
-- java.lang.Exception
--- java.io.IOException
- System.Exception
-- System.SystemException
--- System.IO.IOException
- java.lang.Throwable
-- java.lang.Exception
- java.lang.Throwable
-- java.lang.Exception
--- java.lang.RuntimeException
---- java.lang.IllegalStateException
- java.lang.Throwable
-- java.lang.Error
- System.Exception
-- System.SystemException
- System.Exception
- java.lang.Throwable
- java.lang.Throwable
-- java.lang.Exception
-- java.io.IOException
- System.Exception
-- System.SystemException
--- System.IO.IOException
- java.lang.Throwable
-- java.lang.Exception
--- java.lang.RuntimeException
- System.Exception
-- System.SystemException
- java.lang.Throwable
-- java.lang.Exception
--- java.io.IOException
--- org.apache.lucene.index.CorruptIndexException
- System.Exception
-- System.SystemException
-- System.IO.IOException
- java.lang.Throwable
-- java.lang.Exception
--- java.io.IOException
--- org.apache.lucene.index.CorruptIndexException
- System.Exception
-- System.SystemException
-- System.IO.IOException
- java.lang.Throwable
-- java.lang.Exception
--- java.lang.RuntimeException
- System.Exception
- java.lang.Throwable
-- java.lang.Exception
--- java.lang.RuntimeException
---- java.lang.IllegalStateException
- System.Exception
-- System.SystemException
-- System.InvalidOperationException
- java.lang.Throwable
-- java.lang.Exception
--- java.io.IOException
- System.Exception
-- System.SystemException
-- System.IO.IOException
- java.lang.Throwable
-- java.lang.Exception
--- java.lang.RuntimeException
- System.Exception
- java.lang.Throwable
-- java.lang.Exception
- System.Exception
- java.lang.Throwable
-- java.lang.Error
--- java.lang.VirtualMachineError
- System.Exception
-- System.SystemException
- java.lang.Throwable
-- java.lang.Exception
--- java.lang.RuntimeException
- System.Exception
-- System.SystemException
- java.lang.Throwable
-- java.lang.Exception
--- java.lang.RuntimeException
- System.Exception
- java.lang.Throwable
-- java.lang.Exception
- System.Exception
- java.lang.Throwable
-- java.lang.Error
- System.Exception
- java.lang.Throwable
-- java.lang.Exception
--- java.lang.RuntimeException
- System.Exception
-- System.SystemException
- java.lang.Throwable
-- java.lang.Exception
--- org.apache.lucene.queryparser.flexible.core.QueryNodeException
---- org.apache.lucene.queryparser.flexible.core.QueryNodeParseException
- System.Exception
-- Lucene.Net.QueryParsers.Flexible.Core.QueryNodeException
--- Lucene.Net.QueryParsers.Flexible.Core.QueryNodeParseException
- java.lang.Throwable
-- java.lang.Exception
- System.Exception
-- Lucene.Net.QueryParsers.Flexible.Core.QueryNodeException
- java.lang.Throwable
-- java.lang.Error
- System.Exception
- java.lang.Throwable
-- java.lang.Error
- System.Exception
- java.lang.Throwable
-- java.lang.Exception
- System.Exception
- java.lang.Throwable
-- java.lang.Exception
- System.Exception
- java.lang.Throwable
-- java.lang.Exception
--- java.io.IOException
- System.Exception
-- System.SystemException
-- System.IO.IOException
- java.lang.Throwable
-- java.lang.Error
--- java.lang.LinkageError
- System.Exception
-- System.SystemException
- java.lang.Throwable
-- java.lang.Exception
--- java.lang.RuntimeException
---- org.junit.internal.AssumptionViolatedException
- System.Exception
-- NUnit.Framework.ResultStateException
- java.lang.Throwable
-- java.lang.Exception
--- java.io.IOException
---- java.nio.file.FileSystemException
- System.Exception
-- System.SystemException
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.
java.text.ParseException
FormatException
in some places,InvalidOperationException
in some places, andExcpetion
in other places (there may be more).java.nio.file.AccessDeniedException
System.UnauthorizedAccessException
does not subclassIOException
. As a result, catch blocks forIOException
are missing this one when they should be catching it.java.io.FileNotFoundException
andjava.nio.file.NoSuchFileException
org.apache.lucene.store.NoSuchDirectoryException
System.IO.DirectoryNotFoundException
, however it does not subclassSystem.IO.FileNotFoundException
, so we needed to manually duplicatecatch
blocks everywhereSystem.IO.FileNotFoundExeption
was caught and duplicate error handler logic.java.lang.OutOfMemoryError
java.lang.Error
. This means in .NET when we catchException
, 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
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 asjava.lang.OutOfMemoryError
.java.lang.NullPointerException
null
parameter in .NET, the expected exception isArgumentNullException
rather thanNullReferenceException
.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
Proposed Solution for Catching Exceptions
We can take advantage of the C#
when
in acatch
statement to make complex rules in catch blocks. In addition, we can use extension methods onException
with names that match exception groups in Java to apply the rules specifc to that group.Example
FileNotFoundException Usage
Click to expand
Java
C# Before
C# After
IOException Usage
Click to expand
Java
C# Before
C# After
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
ornull
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
ParseException Usage
Click to expand
Java
C# Before
C# After
The text was updated successfully, but these errors were encountered: