- Notifications
You must be signed in to change notification settings - Fork311
Code Health | Make SqlDependencyProcessDispatcherStorage notunsafe#3208
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Code Health | Make SqlDependencyProcessDispatcherStorage notunsafe#3208
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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)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 If it isn't broken then I don't see this adding value. |
EgorBo commentedMar 11, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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. |
unsafe@Wraith2 I think the lock change stands on its own. Being able to remove 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. |
Like it was mentioned before, the new code will most likely be annotated with |
| Trace.Assert(0 == s_size); // Size should still be zero at this point. | ||
| s_data = Marshal.AllocHGlobal(data.Length); | ||
| Trace.Assert(s_data != IntPtr.Zero); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Even if it's still technically unsafe, less pointer type conversions and better locking still feel worthwhile.
codecovbot commentedMar 24, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov ReportAll modified and coverable lines are covered by tests ✅
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
Flags with carried forward coverage won't be shown.Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ab504ef intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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 anIntPtrwhich is the safe version of avoid*that it was previously stored as. After changing that, we no longer need to use afixedblock to get an unsafe pointer to data.The other improvement is that this PR removes the spin-lock and replaces it with a
lockblock.Testing: Project still builds, and tests are still working.