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.

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

Remove received bytes length check #100619

Merged
merged 9 commits into from
May 14, 2024
Merged

Remove received bytes length check #100619

merged 9 commits into from
May 14, 2024

Conversation

skyoxZ
Copy link
Contributor

@skyoxZ skyoxZ commented Apr 3, 2024

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.

Copy link
Contributor

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

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Apr 3, 2024
@antonfirsov

This comment was marked as resolved.

@skyoxZ
Copy link
Contributor Author

skyoxZ commented Apr 9, 2024

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
Copy link
Member

antonfirsov commented Apr 9, 2024

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
Copy link
Contributor Author

skyoxZ commented Apr 11, 2024

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

@wfurt
Copy link
Member

wfurt commented Apr 22, 2024

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
Copy link
Contributor Author

skyoxZ commented Apr 23, 2024

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.

Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

wfurt commented May 1, 2024

I'm ok if we proceed.

@antonfirsov antonfirsov merged commit 10cba5b into dotnet:main May 14, 2024
83 checks passed
@karelz karelz added this to the 9.0.0 milestone May 14, 2024
@skyoxZ skyoxZ deleted the udp-receive branch May 14, 2024 14:27
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
* Remove received bytes length check

* delete whitespace

* fix typo

* Improve tests

* Update UdpClientTest.cs

* Update UdpClientTest.cs

---------

Co-authored-by: Anton Firszov <Anton.Firszov@microsoft.com>
@github-actions github-actions bot locked and limited conversation to collaborators Jun 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Sockets community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants