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.

Sign up for GitHub

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 | SNI Native Wrapper #3056

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

Cleanup | SNI Native Wrapper #3056

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
abfae65
Cleanup member variables of SniNativeWrapper
benrr101 Nov 13, 2024
5361d6c
Sort public methods (as per target names)
benrr101 Nov 13, 2024
5440bc0
Sort private methods (most of these will be removed in next commit)
benrr101 Nov 13, 2024
9382578
Remove unneeded private methods
benrr101 Nov 14, 2024
1b8614e
Rename methods as per naming rules
benrr101 Nov 14, 2024
9ff384a
Cleanup suggestions and follow naming conventions
benrr101 Nov 14, 2024
87262c5
Remove AppDomain method from SniNativeWrapper
benrr101 Nov 14, 2024
f8c0769
Remove Win32NativeMethods
benrr101 Nov 14, 2024
c0f51c8
Remove sqldependency process storage methods from SniNativeWrapper
benrr101 Nov 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 0 additions & 11 deletions src/Microsoft.Data.SqlClient/src/Interop/Windows/Sni/SniNativeWrapper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,18 +456,7 @@ private static void MarshalConsumerInfo(ConsumerInfo consumerInfo, ref SniConsum

#endregion


#if NETFRAMEWORK
static AppDomain GetDefaultAppDomainInternal()
{
return AppDomain.CurrentDomain;
}

internal static _AppDomain GetDefaultAppDomain()
{
return GetDefaultAppDomainInternal();
}

[ResourceExposure(ResourceScope.Process)] // SxS: there is no way to set scope = Instance, using Process which is wider
[ResourceConsumption(ResourceScope.Process, ResourceScope.Process)]
internal static unsafe byte[] GetData()
Expand Down
45 changes: 18 additions & 27 deletions src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlDependency.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,47 +470,38 @@ private static void ObtainProcessDispatcher()

#if DEBUG // Possibly expensive, limit to debug.
SqlClientEventSource.Log.TryNotificationTraceEvent("<sc.SqlDependency.ObtainProcessDispatcher|DEP> AppDomain.CurrentDomain.FriendlyName: {0}", AppDomain.CurrentDomain.FriendlyName);

#endif // DEBUG
_AppDomain masterDomain = SniNativeWrapper.GetDefaultAppDomain();

if (masterDomain != null)

_AppDomain masterDomain = AppDomain.CurrentDomain;

ObjectHandle handle = CreateProcessDispatcher(masterDomain);
if (handle != null)
{
ObjectHandle handle = CreateProcessDispatcher(masterDomain);
SqlDependencyProcessDispatcher dependency = (SqlDependencyProcessDispatcher)handle.Unwrap();

if (handle != null)
if (dependency != null)
{
SqlDependencyProcessDispatcher dependency = (SqlDependencyProcessDispatcher)handle.Unwrap();

if (dependency != null)
{
s_processDispatcher = SqlDependencyProcessDispatcher.SingletonProcessDispatcher; // Set to static instance.
s_processDispatcher = SqlDependencyProcessDispatcher.SingletonProcessDispatcher; // Set to static instance.

// Serialize and set in native.
using (MemoryStream stream = new())
{
SqlClientObjRef objRef = new(s_processDispatcher);
DataContractSerializer serializer = new(objRef.GetType());
GetSerializedObject(objRef, serializer, stream);
SniNativeWrapper.SetData(stream.ToArray()); // Native will be forced to synchronize and not overwrite.
}
}
else
// Serialize and set in native.
using (MemoryStream stream = new())
{
SqlClientEventSource.Log.TryNotificationTraceEvent("<sc.SqlDependency.ObtainProcessDispatcher|DEP|ERR> ERROR - ObjectHandle.Unwrap returned null!");
throw ADP.InternalError(ADP.InternalErrorCode.SqlDependencyObtainProcessDispatcherFailureObjectHandle);
SqlClientObjRef objRef = new(s_processDispatcher);
DataContractSerializer serializer = new(objRef.GetType());
GetSerializedObject(objRef, serializer, stream);
SniNativeWrapper.SetData(stream.ToArray()); // Native will be forced to synchronize and not overwrite.
}
}
else
{
SqlClientEventSource.Log.TryNotificationTraceEvent("<sc.SqlDependency.ObtainProcessDispatcher|DEP|ERR> ERROR - AppDomain.CreateInstance returned null!");
throw ADP.InternalError(ADP.InternalErrorCode.SqlDependencyProcessDispatcherFailureCreateInstance);
SqlClientEventSource.Log.TryNotificationTraceEvent("<sc.SqlDependency.ObtainProcessDispatcher|DEP|ERR> ERROR - ObjectHandle.Unwrap returned null!");
throw ADP.InternalError(ADP.InternalErrorCode.SqlDependencyObtainProcessDispatcherFailureObjectHandle);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The net change here will result in a different error code (and trace message), but I think the change makes sense

}
}
else
{
SqlClientEventSource.Log.TryNotificationTraceEvent("<sc.SqlDependency.ObtainProcessDispatcher|DEP|ERR> ERROR - unable to obtain default AppDomain!");
throw ADP.InternalError(ADP.InternalErrorCode.SqlDependencyProcessDispatcherFailureAppDomain);
SqlClientEventSource.Log.TryNotificationTraceEvent("<sc.SqlDependency.ObtainProcessDispatcher|DEP|ERR> ERROR - AppDomain.CreateInstance returned null!");
throw ADP.InternalError(ADP.InternalErrorCode.SqlDependencyProcessDispatcherFailureCreateInstance);
}
}
else
Expand Down