Remove received bytes length check

[skyoxZ]

Any packet can't be larger than 65535 bytes so it should never reach to MaxUDPSize(65536). Even if that happens, I don't think it's good to return _buffer directly in case users handle the data asynchronously.

[dotnet-policy-service]

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

[skyoxZ]

The proposed change penalizes all other use-cases with extra allocations which we would like to avoid.

I think only the case of 65536 bytes received got changed. Generally this PR just removes the received == 65536 check (which should always be False). Am I missing something?

[antonfirsov]

Ignore my previous comment, I got confused what's happening here, since I was only looking at the changed codeblocks 😞

The change basically removes an "optimization" for the received == 65536 case which seems to be practically impossible indeed. Maybe the original authors of the code wanted this optimization for received == 65535, but that still seems to be extremely rare, so it makes sense to remove the branch. @wfurt do you agree?

Unfortunately the test cases in UdpClientTest do not seem to validate the contents of the array. I'm fine taking this PR if it would also extends those tests. (Or if the validation is there but I overlooked it.)

[skyoxZ]

Updated tests. Some tests are skipped for my Mac. Hope everything is fine.

[wfurt]

Is there particular functional issue you are trying to fix @skyoxZ? While I do no know why the code exist as it is but I would assume it was put in for some reason. I really wish we could obsolete UDPClient but it is legacy API and and I'm not sure it is worth of touching.

[skyoxZ]

Is there particular functional issue you are trying to fix?

No, just found it when reading the code.

I really wish we could obsolete UDPClient but it is legacy API and I'm not sure it is worth of touching.

I don’t think UDPClient is not recommended any more. It just works without any particular issue (a negative example is SmtpClient, which doesn’t work in many real-world cases).

I’m confident of my proposal but there’s a risk that my codes may have bug. Feel free to close the PR if you prefer to do so.

[antonfirsov]

While I do no know why the code exist as it is but I would assume it was put in for some reason.

It looks like an "optimization" for the case where received == 65536. It prevents a copy since _buffer has the correct size in that case. Given this case is impossible or at least extremely unlikely, I see no harm from removing it.

I really wish we could obsolete UDPClient

Given it's not obsolete yet, I see value in code simplifications. IMO there is no risk from merging this PR & especially that it also adds tests. @wfurt if you have strong concerns, feel free to close the PR, otherwise I will go ahead and merge.

[wfurt]

I'm ok if we proceed.