Remove CER #3535
Remove CER #3535
Conversation
benrr101
added
the
Code Health 💊
There was a problem hiding this comment.
Pull Request Overview
This PR removes Constrained Execution Regions (CER) from the codebase, which have become obsolete and problematic to maintain. CER was originally designed to provide guarantees about code execution in the face of asynchronous exceptions, but has evolved into a best-effort mechanism that is no longer necessary for the project's goals.
Key changes include:
- Removal of
RuntimeHelpers.PrepareConstrainedRegions()calls and associated try/catch/finally blocks - Elimination of specific exception handling for
OutOfMemoryException,StackOverflowException, andThreadAbortExceptionwithin CER blocks - Removal of CER-related attributes like
[ReliabilityContract]
Reviewed Changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| TdsParserStateObject.cs | Removed CER preparation and ReliabilityContract attribute from IncrementPendingCallbacks method |
| TdsParserSessionPool.cs | Removed CER-related comment about ThreadAbort handling |
| TdsParserSafeHandles.Windows.cs | Removed CER preparation from SNIHandle constructor |
| SqlUtil.cs | Removed CER blocks and specific exception handling from task continuation methods |
| SqlTransaction.cs | Removed CER preparation and specific exception handling from transaction operations |
| SqlInternalConnection.cs | Removed CER blocks and BestEffortCleanup methods |
| SqlDependencyListener.cs | Removed CER preparation from impersonation context |
| SqlDependency.cs | Removed CER preparation from Start/Stop methods |
| SqlDelegatedTransaction.cs | Removed CER blocks from transaction operations |
| SqlDataReader.cs | Removed extensive CER blocks from data reading operations |
| SqlCommandBuilder.cs | Removed CER preparation from DeriveParameters method |
| SqlBulkCopy.cs | Removed CER blocks from bulk copy operations |
| LocalDbApi.Windows.cs | Removed CER preparation from LocalDB API calls |
| SqlClientMetrics.cs | Removed ReliabilityContract attribute |
| WaitHandleDbConnectionPool.cs | Removed CER blocks from connection pool operations |
| DbConnectionPoolAuthenticationContext.cs | Removed ReliabilityContract attribute |
| SqlDataSourceEnumeratorNativeHelper.cs | Removed CER preparation from data source enumeration |
| DbConnectionInternal.cs | Removed ReliabilityContract attribute from DoomThisConnection |
| AdapterUtil.cs | Removed ReliabilityContract attribute |
| SniNativeWrapper.cs | Removed CER preparation from packet handling |
| TdsParserStateObjectNative.cs (netfx/netcore) | Removed CER blocks from packet disposal |
| TdsParserStateObject.netfx/netcore.cs | Removed CER preparation and RuntimeHelpers stub |
| TdsParser.netfx/netcore.cs | Removed BestEffortCleanup methods and CER blocks |
| SqlInternalConnectionTds.cs (netfx/netcore) | Removed CER blocks from connection operations |
| SqlConnectionHelper.cs | Removed ReliabilityContract attribute |
| SqlConnection.cs (netfx/netcore) | Removed CER blocks from connection operations |
| SqlCommand.cs (netfx/netcore) | Removed extensive CER blocks from command execution |
Comments suppressed due to low confidence (4)
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserSafeHandles.Windows.cs:204
- Extra closing brace that should be removed. The try/finally block was removed but this closing brace was left behind.
}
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs:2767
- Missing opening brace for the method. The CER block removal appears to have left the method definition incomplete.
{
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParserStateObjectNative.cs:192
- Extra closing brace remaining after CER block removal. This should be removed to maintain proper brace matching.
}
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObjectNative.cs:459
- Extra closing brace remaining after CER block removal. This should be removed to maintain proper brace matching.
}
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlTransaction.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlTransaction.cs
Show resolved
Hide resolved
|
@edwardneal @Wraith2 @ErikEJ Would love to get some feedback on this idea. It's definitely a Change ™️ |
|
I definitely agree that the CERs shouldn't stay as they are; we should either write code which meets the reliability contract (and include the use case in CI, review with the SQL Server behaviour in mind, etc.) or stop advertising it as a capability. There's additional linked documentation which highlights other areas where we might also fall short. This could have an impact on situations where someone is running SqlClient within a SQLCLR environment, but not using the context connection (i.e. a custom CLR stored procedure which instantiates a SqlConnection to a separate SQL Server instance.) I wasn't able to load the netfx SqlClient assembly when I tested in #2838, and at the time it wasn't a supported scenario. I'm in favour of removing the CERs. |
| { | ||
| _activeConnection.Abort(e); | ||
| throw; | ||
| } | ||
| catch (Exception) | ||
| { | ||
| // Similarly, if an exception occurs put the stateObj back into the pool. |
There was a problem hiding this comment.
Should this catch be calling _activeConnection.Abort(e) like the other catches used to?
Same with the other 2 below.
| statistics = SqlStatistics.StartTimer(Statistics); | ||
| InnerConnection.ChangeDatabase(database); | ||
| } | ||
| catch (System.OutOfMemoryException e) | ||
| { | ||
| Abort(e); |
There was a problem hiding this comment.
Should we still be calling Abort(e)?
| @@ -539,9 +539,6 @@ internal SqlInternalConnectionTds( | |||
| _parserLock.Wait(canReleaseFromAnyThread: false); | |||
| ThreadHasParserLockForClose = true; // In case of error, let ourselves know that we already own the parser lock | |||
|
|
|||
| #if NETFRAMEWORK | |||
There was a problem hiding this comment.
Why did these netcore files have #if NETFRAMEWORK in them anyway? Was it an effort to make them similar to the netfx files so the merge would be simpler?
| @@ -573,21 +570,6 @@ internal SqlInternalConnectionTds( | |||
| } | |||
| } | |||
| } | |||
| catch (System.OutOfMemoryException) | |||
| { | |||
| DoomThisConnection(); | |||
There was a problem hiding this comment.
No more doom on an exception?
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlTransaction.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
I wonder if we haven't seen much demand for this because most customers who use a driver in SQLCLR are sticking with System.Data.SqlClient. SDS is a much more compelling option for SQLCLR because it avoids an extra package install. So I wouldn't necessarily take a lack of complaint to mean a lack of usage.
That said, the legacy SQLCLR code is currently holding us back in MDS and is broken. I'm not sure that keeping it in place significantly reduces the complexity of adding true support in the future. We can refer to this PR in the future as a starting point if we do decide to work on supporting it.
Found the following:
| @@ -1411,18 +1411,6 @@ private void BeginExecuteNonQueryInternalReadStage(TaskCompletionSource<object> | |||
| CachedAsyncState.SetActiveConnectionAndResult(completion, nameof(EndExecuteNonQuery), _activeConnection); | |||
| _stateObj.ReadSni(completion); | |||
| } | |||
| // Cause of a possible unstable runtime situation on facing with `OutOfMemoryException` and `StackOverflowException` exceptions, | |||
| // trying to call further functions in the catch of either may fail that should be considered on debuging! | |||
| catch (System.OutOfMemoryException e) | |||
There was a problem hiding this comment.
We can't catch System.StackOverflowException and System.Threading.ThreadAbortException isn't a thing in net core so it makes sense to remove those, but System.OutOfMemoryException is catchable... Can you explain a bit more of your reasoning for removing OOM catches?
There was a problem hiding this comment.
The ability to recover from an OOM is unlikely and even if you somehow manage it without corrupting internal state there isn't a much you can do without the ability to allocate.
|
This is a topic i've discussed with David Engel in the past. My understanding is that M.D.SqlClient is not supported or usable within sql server. The only supported version is S.D.SqlClient. With the deprecation of S.D.SqlClient I'm not sure what this means for the SQLCLR feature but until there is official confirmation that M.D.SqlClient is supported in SQL Server I would not expect it to work and if someone tries it and it does work it may just be luck, not something to rely on. Netcore and now NET is not and will never be a supported host in sql server because much of the work done to improve netcore over netfx was removal of things that SQLCLR needed. Things like being able to terminate at any arbitrary point in execution without corrupting the runtime are simply not possible and were a constant source of bugs. ThreadAbortException, CER and related work were part of this featureset and have been removed. |
Description
This is a fairly major change.
https://learn.microsoft.com/en-us/dotnet/framework/performance/constrained-execution-regions#code-not-permitted-in-cers
For the longest time, and especially as part of this common project merge, we have maintained these Constrained Execution Regions as part of our netfx implementation. However, they've been a major pain to work around, since they involve try/catch/finally structures that aren't needed in netcore, or catching exceptions that are never expected to be caught in netcore. There have been several approaches brought forward to conditionally include the CER structures - some better than others. While working on the SqlCommand merge, I suggested another approach that moves the CER structure to a helper (see #3514). This uncovered a major limitation on CER blocks - that function pointers/delegates cannot be used (see link above).
This prompted a bit more research, and it appears that our codebase is riddled with code that cannot be called from a CER block. Since .NET 2.0, CER has been a guideline instead of a hard rule, and CER execution has become best-effort rather than guaranteed, and as a result much more permissive in what can be prepared/executed. Even with obvious issues in the code, CI has passed for ages, and no users have complained of faults arising from CER failures. As such, the question arises - do we even need CER anymore? The answer to this, so far, seems to be no.
So, in this PR, I searched the codebase for instances of
PrepareConstrainedRegionand just removed the entire try/catch/finally structure where possible.Here's some examples of places where we're doing unapproved things inside CER blocks:
SqlCommand.InternalExecuteNonQuerySqlCommand.ExecuteNonQueryRunExecuteNonQueryTdswhich contains a lambda being called on timeout (among others)SqlCommand.ExecuteReaderSqlConnection.InternalOpenAsyncSqlConnection.OpenAsyncTdsParserStateObject.ReadSniErrorTdsParserStateObject.ProcessSniPacketwhich explicitly allocates a byte arrayThis PR is best viewed with whitespace changes ignored
Issues
Will make remaining work for #1261 easier
Testing
Project still builds, CI will hopefully be sufficient for validation of behavior.