Remove StackOverflow/OutOfMemory checks#4011
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes StackOverflowException and OutOfMemoryException checks from the IsCatchableExceptionType and IsCatchableOrSecurityExceptionType helper methods in AdapterUtil.cs, based on the premise that these exception types cannot be caught at runtime.
Changes:
- Removed
StackOverflowExceptionandOutOfMemoryExceptiontype checks fromIsCatchableExceptionType, which is used by ~60+ call sites across the driver to decide whether to re-throw or handle exceptions. - Removed the same checks from
IsCatchableOrSecurityExceptionType, which is used by ~5 call sites.
| return ((type != typeof(ThreadAbortException)) && | ||
| (type != typeof(NullReferenceException)) && | ||
| (type != typeof(AccessViolationException)) && | ||
| !typeof(SecurityException).IsAssignableFrom(type)); |
There was a problem hiding this comment.
Removing the OutOfMemoryException check is incorrect. Unlike StackOverflowException, OutOfMemoryException is catchable in .NET — both in .NET Framework and .NET Core/.NET 8+. The CLR throws it as a regular managed exception when memory allocation fails (e.g., new byte[int.MaxValue]), and it can be caught in a catch block.
This codebase itself demonstrates this: see ChannelDbConnectionPoolTest.cs:851 where OutOfMemoryException is caught in a test, and ArrayBufferWriter.netfx.cs:263 where OutOfMemoryException is explicitly thrown by user code.
By removing this check, OutOfMemoryException will now be treated as "catchable" by all ~60+ callers of IsCatchableExceptionType. In many of those call sites (e.g., TdsParserStateObject.Deactivate at line 1087), when IsCatchableExceptionType returns true, the exception is swallowed rather than re-thrown. This means OOM conditions could be silently eaten, leading to hard-to-diagnose failures.
The OutOfMemoryException check should be restored. Only the StackOverflowException removal is defensible (since runtime-generated SOEs terminate the process before reaching any catch block).
| (type != typeof(ThreadAbortException)) && | ||
| return ((type != typeof(ThreadAbortException)) && | ||
| (type != typeof(NullReferenceException)) && | ||
| (type != typeof(AccessViolationException))); |
There was a problem hiding this comment.
Same issue as in IsCatchableExceptionType: the OutOfMemoryException check should be restored here. OutOfMemoryException is catchable in .NET and removing this check changes the semantics for callers that use this method to decide whether to re-throw or swallow exceptions.
| (type != typeof(AccessViolationException))); | |
| (type != typeof(AccessViolationException)) && | |
| (type != typeof(OutOfMemoryException))); |
| return ((type != typeof(StackOverflowException)) && | ||
| (type != typeof(OutOfMemoryException)) && | ||
| (type != typeof(ThreadAbortException)) && | ||
| return ((type != typeof(ThreadAbortException)) && |
There was a problem hiding this comment.
The comment still says "only StackOverflowException & ThreadAbortException are sealed classes" but StackOverflowException has been removed from the checks below. If you do proceed with removing StackOverflowException, this comment should be updated to reflect the current set of exceptions being checked (e.g., "only ThreadAbortException is a sealed class").
| return ((type != typeof(StackOverflowException)) && | ||
| (type != typeof(OutOfMemoryException)) && | ||
| (type != typeof(ThreadAbortException)) && | ||
| return ((type != typeof(ThreadAbortException)) && |
There was a problem hiding this comment.
Same stale comment issue: line 410 still says "only StackOverflowException & ThreadAbortException are sealed classes" but StackOverflowException has been removed from the checks. This comment should be updated to match the code.
my understanding is that it is not possible to catch those exception types.
Happy to be educated as to otherwise