Merge | Merge netfx-specific exception handling

[edwardneal]

Contributes to #2965 and #1261.

This PR merges the exception handling for ThreadAbortException, StackOverflowException and OutOfMemoryException between .NET Core and .NET Framework.

Following this PR, it becomes fairly trivial to merge SqlBulkCopy, SqlDataReader and SqlTransaction.

Given the number of indentation changes from try/catch blocks, I'd recommend switching off whitespace when checking the diff - it eliminates around a third of it.

I'd appreciate a CI run for this.

[paulmedynski]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[benrr101]

I'm a bit on the fence about this one, so I'd like to hear your thoughts before approving.

1. My understanding of the constrained execution regions is that checking for OutOfMemoryException, StackOverflowException, ThreadAbortException is only required if there is a constrained execution region. I'm a bit concerned if this is a bit of a behavior change.
2. It looks to me that the plan is to have put the bestEffortCleanup in both netfx and netcore, but make it such that the netcore BestEffortCleanup implementation does nothing. I'm not a huge fan of this, since without digging into the implementations of BestEffortCleanup, it won't be obvious nothing is happening. I get that it'd be easier to merge the codebases, but I'm worried it won't be clear what's actually happening.

This isn't the first time something like this has come up, so I wonder if maybe we should come up with something that makes it clear the method call is only doing anything when in netfx or netcore. Sorta like how Debug.Assert is expected to only run when it's a DEBUG build.

[codecov]

Codecov Report

Attention: Patch coverage is 25.95937% with 328 lines in your changes missing coverage. Please review.

Project coverage is 66.11%. Comparing base (0be2756) to head (178d06f).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
...core/src/Microsoft/Data/SqlClient/SqlDataReader.cs	25.74%	225 Missing ⚠️
...ore/src/Microsoft/Data/SqlClient/SqlTransaction.cs	18.05%	59 Missing ⚠️
...core/src/Microsoft/Data/SqlClient/SqlConnection.cs	14.81%	23 Missing ⚠️
...etcore/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs	41.66%	14 Missing ⚠️
.../netfx/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs	33.33%	6 Missing ⚠️
.../Microsoft/Data/SqlClient/SqlInternalConnection.cs	0.00%	1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (0be2756) and HEAD (178d06f). Click for more details.

HEAD has 1 upload less than BASE

Flag	BASE (0be2756)	HEAD (178d06f)
addons	1	0

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3179      +/-   ##
==========================================
- Coverage   72.81%   66.11%   -6.71%     
==========================================
  Files         282      276       -6     
  Lines       59112    59074      -38     
==========================================
- Hits        43045    39055    -3990     
- Misses      16067    20019    +3952     

Flag	Coverage Δ
addons	?
netcore	68.89% <25.29%> (-6.63%)	⬇️
netfx	65.00% <62.50%> (-6.15%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[edwardneal]

Thanks. The changes at the moment are no-ops, there's no behaviour change. An OutOfMemoryException without CERs means that the CLR's in an undefined state anyway, a StackOverflowException results in the process being killed and netcore doesn't throw ThreadAbortExceptions at all.

What about wrapping every call to BestEffortCleanup in #if NETFRAMEWORK? I hadn't originally wanted to do that, but that was purely cosmetic - I didn't want to leave conditional compilation scattered across the codebase. It'd be a little clearer than having a stub method though.

[benrr101]

... there's no behaviour change...

I think the only instances where was most concerned about behavior change was when the new catches were added when there wasn't a catch-all exception handler. All the other instances are fine since they basically do the same thing as the catch-all handler.

My understanding, too, is that we really only have the CERs in place to handle situations where SqlClient is being called from SQL Server CLR. The theory being that the CER is there to protect SQL Server from crashing if something goes wrong with SqlClient. Outside of those situations, I don't care as much about cleanup behavior. I suppose my primary issue is whether we'll try to clean things up in netcore that were not being cleaned up before.

What about wrapping every call to BestEffortCleanup in #if NETFRAMEWORK?

Yeah.... that's what I had been doing for the merge I've been working on for SqlTransaction. It's messy as all heck, but it's generally clear that the code is being used or not. I've even been wrapping the extra catch blocks in the same #if. One thing I've done (that nobody's complained or applauded yet) is keep the #if tabbed over the same as the code it's wrapping. It makes it a bit less clumsy.

One experiment I was trying (and this prompted removing the resilience sections) was to use an inline function to implement the meat and potatoes of the method and either call it directly (in netcore) or pass it into a method that wraps it with appropriate CES handling. It appears that by using inline functions, it's only marginally less performant, but cleans up the code a lot. Something like:

public void MyMethod()
{
  void DoTheStuff()
  {
    // stuff
  }

  #if NETFRAMEWORK
  callWithCes(DoTheStuff)
  #else
  DoTheStuff();
  #endif
}

of course what would be really nice is if we could just get rid of CERs...

[edwardneal]

I think the only instances where was most concerned about behavior change was when the new catches were added when there wasn't a catch-all exception handler.

I understand. These three exceptions fall into two groups though:

1. Never thrown in netcore (which means the catch is unreachable and any change in behaviour is a moot point)
2. Thrown where the internal state of the netcore CLR is in the process of terminating as a result of a lack of memory (which kills the connection anyway)

Yeah.... that's what I had been doing for the merge I've been working on for SqlTransaction. It's messy as all heck, but it's generally clear that the code is being used or not. I've even been wrapping the extra catch blocks in the same #if.

I'll do that, thanks. I think BestEffortCleanup is the best place to draw the line, since its body relies on details of TdsParserStateObject which can't be merged and the catch blocks are functionally no-ops.