Skip to content
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

Cleanup/Merge | Remove Reliability Section #3042

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

benrr101
Copy link
Contributor

Description: After some discussion with @saurabh500 we came to the conclusion that it's just not beneficial to have the ReliabilitySection code in the netfx project. It is only included in debug builds (which we don't even use for CI builds anyhow), and it makes massive clutter in the code base. Although I've experimented with some ideas to leave it in place but cleanup the code, simply removing it is probably the easiest solution.

Testing: Project still builds without it and since it provides no functionality, it won't break anything to remove it.

@benrr101 benrr101 added ➕ Code Health Issues/PRs that are targeted to source code quality improvements. Common Project 🚮 Things that relate to the common project project labels Nov 26, 2024
@edwardneal
Copy link
Contributor

This looks good to me. Are the TdsParser.ReliabilitySection struct definitions due to be removed in a future PR?

@MichelZ
Copy link
Contributor

MichelZ commented Nov 27, 2024

These can be deleted as well in netcore SqlDataReader:

#if NETFRAMEWORK && DEBUG
TdsParser.ReliabilitySection tdsReliabilitySection = new TdsParser.ReliabilitySection();
RuntimeHelpers.PrepareConstrainedRegions();
try
{
tdsReliabilitySection.Start();
#else

(and many others in there)

As well as

#if DEBUG
TdsParser.ReliabilitySection tdsReliabilitySection = new TdsParser.ReliabilitySection();
RuntimeHelpers.PrepareConstrainedRegions();
try
{
tdsReliabilitySection.Start();
#endif //DEBUG

(and others in there)

and of course the defitinition

// ReliabilitySection Usage:
//
// #if DEBUG
// TdsParser.ReliabilitySection tdsReliabilitySection = new TdsParser.ReliabilitySection();
//
// RuntimeHelpers.PrepareConstrainedRegions();
// try {
// tdsReliabilitySection.Start();
// #else
// {
// #endif //DEBUG
//
// // code that requires reliability
//
// }
// #if DEBUG
// finally {
// tdsReliabilitySection.Stop();
// }
// #endif //DEBUG
internal struct ReliabilitySection
{
#if DEBUG
// do not allocate TLS data in RETAIL bits
[ThreadStatic]
private static int s_reliabilityCount; // initialized to 0 by CLR
private bool m_started; // initialized to false (not started) by CLR
#endif //DEBUG
[Conditional("DEBUG")]
internal void Start()
{
#if DEBUG
Debug.Assert(!m_started);
RuntimeHelpers.PrepareConstrainedRegions();
try
{
}
finally
{
++s_reliabilityCount;
m_started = true;
}
#endif //DEBUG
}
[Conditional("DEBUG")]
internal void Stop()
{
#if DEBUG
// cannot assert m_started - ThreadAbortException can be raised before Start is called
if (m_started)
{
Debug.Assert(s_reliabilityCount > 0);
RuntimeHelpers.PrepareConstrainedRegions();
try
{
}
finally
{
--s_reliabilityCount;
m_started = false;
}
}
#endif //DEBUG
}
// you need to setup for a thread abort somewhere before you call this method
[Conditional("DEBUG")]
internal static void Assert(string message)
{
#if DEBUG
Debug.Assert(s_reliabilityCount > 0, message);
#endif //DEBUG
}
}

Copy link

codecov bot commented Nov 27, 2024

Codecov Report

Attention: Patch coverage is 62.36453% with 382 lines in your changes missing coverage. Please review.

Project coverage is 72.66%. Comparing base (46e8714) to head (1c61d33).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
...core/src/Microsoft/Data/SqlClient/SqlDataReader.cs 56.84% 123 Missing ⚠️
...etfx/src/Microsoft/Data/SqlClient/SqlDataReader.cs 57.04% 122 Missing ⚠️
...t/netfx/src/Microsoft/Data/SqlClient/SqlCommand.cs 62.10% 83 Missing ⚠️
...icrosoft/Data/SqlClient/SqlDelegatedTransaction.cs 59.32% 24 Missing ⚠️
...nt/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs 43.75% 18 Missing ⚠️
...etfx/src/Microsoft/Data/SqlClient/SqlConnection.cs 74.28% 9 Missing ⚠️
...crosoft/Data/SqlClient/SqlInternalConnectionTds.cs 91.66% 2 Missing ⚠️
...fx/src/Microsoft/Data/SqlClient/TdsParser.netfx.cs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3042   +/-   ##
=======================================
  Coverage   72.66%   72.66%           
=======================================
  Files         283      283           
  Lines       58975    58909   -66     
=======================================
- Hits        42853    42806   -47     
+ Misses      16122    16103   -19     
Flag Coverage Δ
addons 92.58% <ø> (ø)
netcore 75.44% <60.16%> (+<0.01%) ⬆️
netfx 71.12% <64.52%> (-0.01%) ⬇️

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.

@benrr101 benrr101 force-pushed the merge/reliabilitysection-delete branch from 73caecc to c099dab Compare November 27, 2024 23:19
@cheenamalhotra cheenamalhotra added this to the 7.0-preview1 milestone Dec 3, 2024
@MichelZ
Copy link
Contributor

MichelZ commented Dec 17, 2024

@cheenamalhotra @mdaigle Can this be merged? That would help with #2953

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Common Project 🚮 Things that relate to the common project project ➕ Code Health Issues/PRs that are targeted to source code quality improvements.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants