Tagging subscribers to this area: @karelz, @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

🤖 Copilot Code Review — PR #124482

Holistic Assessment

Motivation: The issue is real and well-documented (#118055). UdpClient.CheckForBroadcast() silently overrides an explicit EnableBroadcast = false by auto-enabling broadcast when sending to IPAddress.Broadcast. This violates user expectations — if you explicitly disable a feature, it should stay disabled.

Approach: Tracking whether the user explicitly set EnableBroadcast via a private _isBroadcastSetByUser field is a simple, correct, and minimal fix. It follows the same pattern as the existing _isBroadcast field.

Summary: ⚠️ Needs Changes. The core logic is correct and the approach is sound, but the tests won't compile due to a non-existent PlatformDetection property, and there are a few test convention issues.

Detailed Findings

❌ Test compilation error — PlatformDetection.IsThreadingSupported does not exist

All three new test methods use:

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]

This property does not exist on PlatformDetection. The correct property is IsMultithreadingSupported, which is what every other test in this file uses. This will cause a compile error.

Flagged independently by all review models (high confidence).

⚠️ Redundant [SkipOnPlatform(TestPlatforms.Wasi)] attribute

The new tests use both [ConditionalFact(...IsMultithreadingSupported)] and [SkipOnPlatform(TestPlatforms.Wasi)]. Since WASI does not support multithreading, IsMultithreadingSupported already handles the WASI skip. Existing Send-based tests in this file (e.g., BeginSend_AsyncOperationCompletes_Success, Send_InvalidArguments_Throws) use only [ConditionalFact(...IsMultithreadingSupported)] without the Wasi skip. The new tests should follow the same pattern for consistency.

💡 Test consolidation — prefer [Theory] over duplicate [Fact] methods

EnableBroadcast_ExplicitlyDisabled_NotAutoEnabled and EnableBroadcast_ExplicitlyEnabledThenDisabled_NotAutoEnabled test the same outcome with slightly different setup. These could be consolidated into a single [Theory]:

[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsMultithreadingSupported))]
[InlineData(false)] // Set false only
[InlineData(true)]  // Set true then false
public void EnableBroadcast_ExplicitlyDisabled_NotAutoEnabled(bool setTrueFirst)
{
    using (var udpClient = new UdpClient())
    {
        if (setTrueFirst) udpClient.EnableBroadcast = true;
        udpClient.EnableBroadcast = false;

        try
        {
            udpClient.Send(new byte[1], 1, new IPEndPoint(IPAddress.Broadcast, UnusedPort));
        }
        catch (SocketException) { }

        Assert.False(udpClient.EnableBroadcast);
    }
}

✅ Implementation logic — correct and minimal

The _isBroadcastSetByUser flag correctly distinguishes between "user never touched EnableBroadcast" (auto-enable preserved for backward compatibility) and "user explicitly set EnableBroadcast" (user's choice respected). The flag is checked in the right place — CheckForBroadcast, which is called from all 7 send/connect paths.

✅ Backward compatibility — preserved for default case

When EnableBroadcast is never explicitly set by the user, the auto-enable behavior is unchanged. The test EnableBroadcast_NotExplicitlySet_AutoEnabled correctly validates this.

💡 Note on Client property replacement

As the PR description mentions, neither _isBroadcast nor _isBroadcastSetByUser is reset when the underlying socket is replaced via the Client property setter. This is a pre-existing gap, and keeping consistent behavior is reasonable. A follow-up could address both fields together.

Addressed the feedback — consolidated the two "explicitly disabled" tests into a [ConditionalTheory] with [InlineData(false/true)] and removed the redundant [SkipOnPlatform(TestPlatforms.Wasi)] attributes, since IsThreadingSupported already returns false on WASI.

One note: PlatformDetection.IsThreadingSupported does exist (PlatformDetection.cs, line 142) and is what every other test in UdpClientTest.cs uses. IsMultithreadingSupported doesn't exist in the codebase. I believe the Copilot review hallucinated that property name.

Also rebased on latest main.

Correction on my previous comment: the Copilot review was right — the property is IsMultithreadingSupported, not IsThreadingSupported. I was mistaken. Fixed in the latest push.

@dotnet-policy-service agree

Hi — just checking in. I addressed all the review feedback on March 5 (consolidated tests into a [ConditionalTheory], removed redundant [SkipOnPlatform], and corrected the PlatformDetection property). Is there anything else needed from my side?

Hi — just checking in. I addressed all the review feedback on March 5 (consolidated tests into a [ConditionalTheory], removed redundant [SkipOnPlatform], and corrected the PlatformDetection property). Is there anything else needed from my side?

Hello, thanks for checking in, no action is needed from your side for now. I'll review this PR today.

/azp run runtime-libraries-coreclr outerloop

/ba-g failures unrelated