Code Health | Make SqlDependencyProcessDispatcherStorage not unsafe

[benrr101]

Description: Fairly small change, but worthy of its own PR. This PR changes the SqlDependencyProcessDispatcherStorage class to be safe (and not unsafe). The data for the property is now stored as an IntPtr which is the safe version of a void* that it was previously stored as. After changing that, we no longer need to use a fixed block to get an unsafe pointer to data.

The other improvement is that this PR removes the spin-lock and replaces it with a lock block.

Testing: Project still builds, and tests are still working.

[Copilot]

PR Overview

This PR aims to improve the safety of the SqlDependencyProcessDispatcherStorage class by replacing the unsafe pointer manipulation with a safe IntPtr-based approach and updating the locking mechanism. Key changes include:

• Removing the unsafe keyword and converting s_data from void* to IntPtr.
• Replacing the spin-lock based synchronization with a lock on a dedicated lock object.
• Updating memory allocation and copying logic to use safe methods.

Reviewed Changes

File	Description
src/Microsoft.Data.SqlClient/src/Interop/Windows/Sni/SqlDependencyProcessDispatcherStorage.netfx.cs	Updated to use IntPtr for safe pointer handling and replaced spin-lock with lock-based synchronization

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

src/Microsoft.Data.SqlClient/src/Interop/Windows/Sni/SqlDependencyProcessDispatcherStorage.netfx.cs:40

• NativeSetData allocates memory only when s_data is IntPtr.Zero. Consider documenting or handling the case when NativeSetData is called again with a data array of a different size to avoid potential memory inconsistencies.

if (s_data == IntPtr.Zero)

[Wraith2]

The new version is still using pointers and not safehandles. The difference between void* and IntPtr seems academic. The code is as safe as it was before it simply lacks the unsafe keyword but that doesn't make it better (or worse) simply different. In a future version of the runtime the new version may still require an unsafe context.

If it isn't broken then I don't see this adding value.

[EgorBo]

While I agree that this PR in fact doesn't make code any safer, the hand-made SpinLock implementation is indeed better to be replaced by a lock IMO. The lock itself is optimized for cases without no contention (thin-lock) and it also does a little of spin-waiting under the hood first. The hand-made spin wait with Sleep(50) does look like it could cause performance/latency troubles.

[benrr101]

@Wraith2 I think the lock change stands on its own. Being able to remove unsafe keywords seems like an improvement to me, but maybe it's six of one, half dozen of the other.

The reason for not calling the lock change out in the first place was the code changes have been sitting around for a long time waiting for other PRs to go through. When I finally got to submitting it, I didn't really remember all the changes I made and only skimmed the diff.

[EgorBo]

Being able to remove unsafe keywords seems like an improvement to me,

Like it was mentioned before, the new code will most likely be annotated with unsafe anyway, see dotnet/designs#330

[paulmedynski]

I don't think this Assert() is helpful. AllocHGlobal() either allocates memory successfully and returns an IntPtr to it, or it throws. Granted, the AllocHGlobal() docs don't explicitly say the returned IntPtr.Zero will never be true, but so what if it is? Will Copy() work if s_data.Zero is true? Those docs don't say either way, so I'm not sure we should be making any assumptions here. I think this Assert() gives a false sense of program state below (i.e. s_data.Zero == false). If it's important that we never call Copy() with a Zero IntPtr, then we need an if-statement.

This is just FYI - no need to make any changes - just wanted to point it out.

[mdaigle]

Even if it's still technically unsafe, less pointer type conversions and better locking still feel worthwhile.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.59%. Comparing base (1e59b88) to head (7bc41fe).
Report is 19 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3208      +/-   ##
==========================================
+ Coverage   72.58%   72.59%   +0.01%     
==========================================
  Files         289      289              
  Lines       59503    59503              
==========================================
+ Hits        43188    43197       +9     
+ Misses      16315    16306       -9     

Flag	Coverage Δ
addons	92.58% <ø> (ø)
netcore	75.15% <ø> (+0.03%)	⬆️
netfx	71.19% <ø> (-0.02%)	⬇️

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.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.