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.

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