-
Notifications
You must be signed in to change notification settings - Fork 0
.pr_agent_auto_best_practices
Pattern 1: Ensure robust network connection handling by disposing wait handles, closing clients on failure, and wrapping EndConnect in try/catch to prevent resource leaks and surface late SocketExceptions with detailed logs.
Example code before:
var result = client.BeginConnect(ip, port, null, null);
if (!result.AsyncWaitHandle.WaitOne(TimeSpan.FromSeconds(5))) return false;
client.EndConnect(result); // may throw late
Example code after:
var ar = client.BeginConnect(ip, port, null, null);
var wh = ar.AsyncWaitHandle;
try
{
if (!wh.WaitOne(TimeSpan.FromSeconds(5)))
{
logger.Error($"Timeout connecting to {ip}:{port}");
client.Close();
return false;
}
try { client.EndConnect(ar); }
catch (SocketException ex)
{
logger.Error(ex, $"Connect failed (SocketError={ex.SocketErrorCode})");
client.Close();
return false;
}
}
finally { wh.Close(); }
Relevant past accepted suggestions:
Suggestion 1:
Prevent connection handle leaks
Ensure the asynchronous wait handle is disposed and the client is closed on failure to avoid resource leaks. Also guard EndConnect with a try/catch to handle late connection failures cleanly and log them.
Daqifi.Desktop/Device/WiFiDevice/DaqifiStreamingDevice.cs [40-54]
Client = new TcpClient();
var result = Client.BeginConnect(IpAddress, Port, null, null);
-var success = result.AsyncWaitHandle.WaitOne(TimeSpan.FromSeconds(5));
+var waitHandle = result.AsyncWaitHandle;
+try
+{
+ var success = waitHandle.WaitOne(TimeSpan.FromSeconds(5));
+ if (!success)
+ {
+ AppLogger.Error($"Timeout connecting to DAQiFi device at {IpAddress}:{Port}");
+ Client.Close();
+ return false;
+ }
-if (!success)
+ // Complete the asynchronous connection
+ try
+ {
+ Client.EndConnect(result);
+ }
+ catch (SocketException ex)
+ {
+ AppLogger.Error(ex, $"Failed to connect to DAQiFi device at {IpAddress}:{Port}");
+ Client.Close();
+ return false;
+ }
+}
+finally
{
- AppLogger.Error($"Timeout connecting to DAQiFi device at {IpAddress}:{Port}");
- return false;
+ waitHandle.Close();
}
-// Complete the asynchronous connection
-Client.EndConnect(result);
+MessageProducer = new MessageProducer(Client.GetStream());
+MessageProducer.Start()
-MessageProducer = new MessageProducer(Client.GetStream());
-MessageProducer.Start();
-Suggestion 2:
Log full socket exception
Log richer context by including the socket error code and full exception to aid troubleshooting. This preserves diagnostic detail without changing control flow.
Daqifi.Desktop/Device/WiFiDevice/DaqifiDeviceFinder.cs [74-77]
catch (SocketException sockEx)
{
- AppLogger.Warning($"Error sending broadcast to {endpoint}: {sockEx.Message}");
+ AppLogger.Warning($"Error sending broadcast to {endpoint} (SocketError={sockEx.SocketErrorCode}): {sockEx}");
}Pattern 2: Harden COM interop by narrowing catch blocks to expected COMError codes, validating inputs, avoiding duplicate operations, and always releasing COM objects in finally to prevent leaks and masked failures.
Example code before:
var policy = GetPolicy();
var rules = policy.Rules;
try
{
var rule = Activator.CreateInstance(Type.GetTypeFromProgID("HNetCfg.FWRule")!);
policy.Rules.Add(rule); // may throw or duplicate
}
catch
{
// swallow
}
Example code after:
if (RuleExists(name)) return;
if (string.IsNullOrWhiteSpace(appPath) || !File.Exists(appPath))
throw new FileNotFoundException("Invalid app path", appPath);
dynamic? policy = null, rules = null, rule = null;
try
{
policy = GetPolicy();
rules = policy.Rules;
var ruleType = Type.GetTypeFromProgID("HNetCfg.FWRule")
?? throw new InvalidOperationException("FWRule COM type missing");
rule = Activator.CreateInstance(ruleType)!;
rule.Name = name; rule.ApplicationName = appPath; rule.Protocol = 17; rule.Direction = 1;
rule.LocalPorts = "38230"; rule.Action = 1; rule.Enabled = true; rule.Profiles = 7;
rules.Add(rule);
}
catch (COMException ex) when ((uint)ex.ErrorCode == 0x80070005 || (uint)ex.ErrorCode == 0x80070035)
{
logger.Warning(ex, "Firewall rule creation failed");
}
finally
{
if (rule != null) Marshal.FinalReleaseComObject(rule);
if (rules != null) Marshal.FinalReleaseComObject(rules);
if (policy != null) Marshal.FinalReleaseComObject(policy);
}
Relevant past accepted suggestions:
Suggestion 1:
Don't mask critical COM errors
Avoid swallowing unexpected exceptions as "rule not found." Only treat a specific COM error indicating missing rule as false, and let others bubble up or be logged. This prevents masking real failures (e.g., access denied) that should stop execution.
Daqifi.Desktop/Configuration/FirewallManager.cs [98-118]
public bool RuleExists(string ruleName)
{
+ dynamic policy = GetPolicy();
+ dynamic rules = policy.Rules;
try
{
- dynamic policy = GetPolicy();
- dynamic rules = policy.Rules;
- try
- {
- var _ = rules.Item(ruleName);
- return true;
- }
- catch (COMException)
- {
- return false;
- }
+ var _ = rules.Item(ruleName);
+ return true;
}
- catch
+ catch (COMException ex) when ((uint)ex.ErrorCode == 0x80070002 /* ERROR_FILE_NOT_FOUND */)
{
return false;
}
}Suggestion 2:
Handle COM failures and duplicates
Guard against duplicate rule creation and COM failures to prevent crashes when the app lacks elevation or the rule already exists. Wrap COM calls in try/catch and release COM objects in a finally block to avoid leaks.
Daqifi.Desktop/Configuration/FirewallManager.cs [120-136]
public void CreateUdpRule(string ruleName, string applicationPath)
{
- dynamic policy = GetPolicy();
- var ruleType = Type.GetTypeFromProgID("HNetCfg.FWRule")
- ?? throw new InvalidOperationException("HNetCfg.FWRule COM type not available");
- dynamic rule = Activator.CreateInstance(ruleType)!;
+ // Avoid duplicate adds which may throw
+ if (RuleExists(ruleName))
+ {
+ return;
+ }
- rule.Name = ruleName;
- rule.ApplicationName = applicationPath;
- rule.Protocol = 17; // UDP
- rule.Direction = 1; // Inbound
- rule.Action = 1; // Allow
- rule.Enabled = true;
- rule.Profiles = 7; // Domain|Private|Public
+ dynamic? policy = null;
+ dynamic? rule = null;
+ try
+ {
+ policy = GetPolicy();
+ var ruleType = Type.GetTypeFromProgID("HNetCfg.FWRule")
+ ?? throw new InvalidOperationException("HNetCfg.FWRule COM type not available");
+ rule = Activator.CreateInstance(ruleType)!;
- policy.Rules.Add(rule);
+ rule.Name = ruleName;
+ rule.ApplicationName = applicationPath;
+ rule.Protocol = 17; // UDP
+ rule.Direction = 1; // Inbound
+ rule.Action = 1; // Allow
+ rule.Enabled = true;
+ rule.Profiles = 7; // Domain|Private|Public
+
+ policy.Rules.Add(rule);
+ }
+ catch (COMException)
+ {
+ // Access denied or rule conflict; fail gracefully without crashing
+ }
+ finally
+ {
+ if (rule != null) Marshal.FinalReleaseComObject(rule);
+ if (policy != null) Marshal.FinalReleaseComObject(policy);
+ }
}Suggestion 3:
Release COM objects and tighten catches
Avoid swallowing all exceptions and ensure COM objects are released to prevent leaks. Narrow the catch to the expected COM-not-found case and clean up COM references in a finally block.
Daqifi.Desktop/Configuration/FirewallManager.cs [98-118]
public bool RuleExists(string ruleName)
{
+ dynamic? policy = null;
+ dynamic? rules = null;
try
{
- dynamic policy = GetPolicy();
- dynamic rules = policy.Rules;
+ policy = GetPolicy();
+ rules = policy.Rules;
try
{
var _ = rules.Item(ruleName);
return true;
}
catch (COMException)
{
+ // Rule not found or no permission to fetch by name
return false;
}
}
- catch
+ finally
{
- return false;
+ if (rules != null) Marshal.FinalReleaseComObject(rules);
+ if (policy != null) Marshal.FinalReleaseComObject(policy);
}
}Suggestion 4:
Restrict firewall rule scope
Explicitly scope the rule to the local UDP port used by the app to avoid opening all UDP traffic. Also validate that applicationPath points to an existing executable before creating the rule to prevent invalid firewall entries.
Daqifi.Desktop/Configuration/FirewallManager.cs [120-136]
public void CreateUdpRule(string ruleName, string applicationPath)
{
+ if (string.IsNullOrWhiteSpace(applicationPath) || !System.IO.File.Exists(applicationPath))
+ throw new FileNotFoundException("Application path for firewall rule is invalid.", applicationPath);
+
dynamic policy = GetPolicy();
var ruleType = Type.GetTypeFromProgID("HNetCfg.FWRule")
?? throw new InvalidOperationException("HNetCfg.FWRule COM type not available");
dynamic rule = Activator.CreateInstance(ruleType)!;
rule.Name = ruleName;
rule.ApplicationName = applicationPath;
rule.Protocol = 17; // UDP
+ rule.LocalPorts = "38230"; // restrict to required UDP port(s)
rule.Direction = 1; // Inbound
rule.Action = 1; // Allow
rule.Enabled = true;
rule.Profiles = 7; // Domain|Private|Public
policy.Rules.Add(rule);
}Pattern 3: Normalize and validate external inputs (e.g., strings and device handles) before logic by trimming and null/whitespace checks, and guarding property access with try/catch to avoid crashes and logical misclassification.
Example code before:
DeviceType type = msg.DevicePn.ToLowerInvariant() switch { "nq1" => ..., _ => Unknown };
return Port.PortName; // can throw or be null/empty
Example code after:
var pn = msg.DevicePn?.Trim();
if (string.IsNullOrWhiteSpace(pn)) { /* keep existing type */ }
else DeviceType = pn.ToLowerInvariant() switch { "nq1" => ..., _ => Unknown };
try { var name = Port?.PortName; return string.IsNullOrWhiteSpace(name) ? "USB" : name; }
catch { return "USB"; }
Relevant past accepted suggestions:
Suggestion 1:
Trim part number before comparison
Add .Trim() to the message.DevicePn string before the switch expression to correctly handle part numbers with leading or trailing whitespace.
Daqifi.Desktop/Device/AbstractStreamingDevice.cs [880-885]
-DeviceType = message.DevicePn.ToLowerInvariant() switch
+DeviceType = message.DevicePn.Trim().ToLowerInvariant() switch
{
"nq1" => DeviceType.Nyquist1,
"nq3" => DeviceType.Nyquist3,
_ => DeviceType.Unknown
};Suggestion 2:
Add test for whitespace-only part numbers
Add a new unit test to verify that a part number consisting only of whitespace does not change the device type, improving coverage for the string.IsNullOrWhiteSpace check.
Daqifi.Desktop.Test/Device/AbstractStreamingDeviceTests.cs [201-217]
[TestMethod]
public void HydrateDeviceMetadata_ShouldNotChangeDeviceTypeWhenPartNumberIsEmpty()
{
// Arrange
var device = new TestStreamingDevice();
device.DeviceType = DeviceType.Nyquist1; // Set initial value
var message = new DaqifiOutMessage
{
DevicePn = ""
};
// Act
device.CallHydrateDeviceMetadata(message);
// Assert
Assert.AreEqual(DeviceType.Nyquist1, device.DeviceType, "Should not change DeviceType when part number is empty");
}
+[TestMethod]
+public void HydrateDeviceMetadata_ShouldNotChangeDeviceTypeWhenPartNumberIsWhitespace()
+{
+ // Arrange
+ var device = new TestStreamingDevice();
+ device.DeviceType = DeviceType.Nyquist1; // Set initial value
+ var message = new DaqifiOutMessage
+ {
+ DevicePn = " "
+ };
+
+ // Act
+ device.CallHydrateDeviceMetadata(message);
+
+ // Assert
+ Assert.AreEqual(DeviceType.Nyquist1, device.DeviceType, "Should not change DeviceType when part number is whitespace");
+}
+Suggestion 3:
Harden COM port retrieval
Guard against disposed or invalid SerialPort access to prevent binding exceptions. Safely read and normalize the port name, falling back to "USB" on null/whitespace or any exception.
Daqifi.Desktop/Device/SerialDevice/SerialStreamingDevice.cs [336-342]
/// <summary>
/// Returns the COM port name for this USB device
/// </summary>
protected override string GetUsbDisplayIdentifier()
{
- return Port?.PortName ?? "USB";
+ try
+ {
+ var name = Port?.PortName;
+ return string.IsNullOrWhiteSpace(name) ? "USB" : name;
+ }
+ catch
+ {
+ return "USB";
+ }
}Pattern 4: Reduce duplication and improve correctness by using a single source of truth for shared fields and aligning versions/types across layers; add precise casts where enums diverge between domains.
Example code before:
core.Direction = deviceDirection;
Name = name;
Index = channelId;
Direction = deviceDirection; // different enum type
Example code after:
core.Direction = (CoreDir)deviceDirection;
Name = core.Name;
Index = core.Id;
Direction = (DeviceDir)core.Direction
Relevant past accepted suggestions:
Suggestion 1:
Align Entity Framework package versions
Update the Microsoft.EntityFrameworkCore.Sqlite and Microsoft.EntityFrameworkCore.Tools packages to version 9.0.10 to align with the Microsoft.EntityFrameworkCore package version.
Daqifi.Desktop/Daqifi.Desktop.csproj [66-68]
<PackageReference Include="Microsoft.EntityFrameworkCore" Version="9.0.10" />
-<PackageReference Include="Microsoft.EntityFrameworkCore.Sqlite" Version="9.0.9" />
-<PackageReference Include="Microsoft.EntityFrameworkCore.Tools" Version="9.0.9">
+<PackageReference Include="Microsoft.EntityFrameworkCore.Sqlite" Version="9.0.10" />
+<PackageReference Include="Microsoft.EntityFrameworkCore.Tools" Version="9.0.10">Suggestion 2:
Fix type mismatch and data duplication
Add an explicit cast for the direction enum to fix a type mismatch and refactor the constructor to use the _coreChannel as the single source of truth for shared properties like Name and Index to prevent data duplication.
Daqifi.Desktop/Channel/AnalogChannel.cs [56-82]
public AnalogChannel(IStreamingDevice owner, string name, int channelId, ChannelDirection direction, bool isBidirectional, float calibrationBValue, float calibrationMValue, float interalScaleMValue, float portRange, uint resolution)
{
_owner = owner;
// Create core channel for device communication
_coreChannel = new Daqifi.Core.Channel.AnalogChannel(channelId, resolution)
{
Name = name,
- Direction = direction,
+ Direction = (Daqifi.Core.Channel.ChannelDirection)direction,
CalibrationB = calibrationBValue,
CalibrationM = calibrationMValue,
InternalScaleM = interalScaleMValue,
PortRange = portRange,
IsEnabled = false
};
- // Set desktop-specific properties
- Name = name;
+ // Set desktop-specific properties, using core channel as the source of truth
+ Name = _coreChannel.Name;
DeviceName = owner.DevicePartNumber;
DeviceSerialNo = owner.DeviceSerialNo;
- Index = channelId;
- Direction = direction;
- IsOutput = direction == ChannelDirection.Output;
+ Index = _coreChannel.Id;
+ Direction = (ChannelDirection)_coreChannel.Direction;
+ IsOutput = Direction == ChannelDirection.Output;
HasAdc = !IsOutput;
IsBidirectional = isBidirectional;
ChannelColorBrush = ChannelColorManager.Instance.NewColor();
}Suggestion 3:
Fix incorrect serial number assignment
In the DigitalChannel constructor, correct the assignment of DeviceSerialNo to use owner.DeviceSerialNo instead of the incorrect owner.DevicePartNumber.
Daqifi.Desktop/Channel/DigitalChannel.cs [104-107]
DeviceName = owner.DevicePartNumber;
-DeviceSerialNo = owner.DevicePartNumber;
+DeviceSerialNo = owner.DeviceSerialNo;
IsBidirectional = isBidirectional;
ChannelColorBrush = ChannelColorManager.Instance.NewColor();[Auto-generated best practices - 2025-11-14]