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

[release/5.0] Fix Socket.ReceiveFrom regression on Unix #46629

Merged
merged 4 commits into from
Jan 14, 2021

Conversation

antonfirsov
Copy link
Member

@antonfirsov antonfirsov commented Jan 6, 2021

Backport of #46151 to release/5.0

Fixes: #45651

Customer Impact

Reported by customer.
On Unix, when using blocking overloads of Socket.ReceiveFrom (typically used in UDP scenarios) after an asynchronous call to the same socket, the call may return wrong remote EndPoint (remoteEP).
A DNS server project hit this incompatibility (see #46151). We expect more UDP scenarios to hit it (typically on the server-side).

Regression?

Yes. The behavior is correct on 3.1, it's a regression introduced in 5.0.

Testing

New targeted test case included in the PR.

Risk

Low. The bug was a small overlook in perf refactoring in the space during 5.0. The fix is minimal, and impacts only the affected sync-over-async ReceiveFrom path.

@ghost
Copy link

ghost commented Jan 6, 2021

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

Issue Details

Backport of #46151 to release/5.0

Fixes: #45651

Customer Impact

On Unix, when using blocking overloads of Socket.ReceiveFrom after an asynchronous call to the same socket, the call may return wrong remoteEP.

Regression?

Yes. The behavior is correct on 3.1.

Testing

Extended SocketTestHelper so existing tests can validate the sync-over-async path of the blocking ReceiveFrom. Unfortunately, those tests are disabled on the 5.0 branch, so I had to port a minimal subset of #44591 (fix enabling the tests) so we can run them on CI for 5.0.

Risk

Low. The bug was a small overlook, the fix is trivial, that impacts only the affected sync-over-async ReceiveFrom path.

Author: antonfirsov
Assignees: -
Labels:

area-System.Net

Milestone: -

@antonfirsov

This comment has been minimized.

@azure-pipelines

This comment has been minimized.

@antonfirsov antonfirsov requested review from geoffkizer and wfurt and removed request for geoffkizer January 6, 2021 14:52
@geoffkizer geoffkizer added this to the 5.0.x milestone Jan 6, 2021
@ghost
Copy link

ghost commented Jan 6, 2021

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

Issue Details

Backport of #46151 to release/5.0

Fixes: #45651

Customer Impact

On Unix, when using blocking overloads of Socket.ReceiveFrom after an asynchronous call to the same socket, the call may return wrong remoteEP. Users of the Socket type working with UDP may hit this.

Regression?

Yes. The behavior is correct on 3.1.

Testing

In the original PR I extended SocketTestHelper so our existing SendToRecvFrom_Datagram_UDP tests can validate the sync-over-async path of the blocking ReceiveFrom. Unfortunately, those tests are disabled on the 5.0 branch, so I had to port a minimal subset of #44591 (the fix enabling the tests) so we can run them on CI for 5.0.

Risk

Low. The bug was a small overlook, the product code change is minimal, and impacts only the affected sync-over-async ReceiveFrom path.

Author: antonfirsov
Assignees: -
Labels:

area-System.Net, area-System.Net.Sockets

Milestone: 5.0.x

@antonfirsov

This comment has been minimized.

@azure-pipelines

This comment has been minimized.

@antonfirsov antonfirsov added Servicing-consider Issue for next servicing release review bug regression-from-last-release labels Jan 7, 2021
@antonfirsov

This comment has been minimized.

@antonfirsov

This comment has been minimized.

@azure-pipelines

This comment has been minimized.

1 similar comment
@azure-pipelines

This comment has been minimized.

@antonfirsov
Copy link
Member Author

Unrelated test failure is #43981.

@karelz
Copy link
Member

karelz commented Jan 12, 2021

@danmosemsft this is ready for 5.0 servicing ...

@leecow leecow removed the Servicing-consider Issue for next servicing release review label Jan 12, 2021
@leecow leecow added the Servicing-approved Approved for servicing release label Jan 12, 2021
@leecow leecow modified the milestones: 5.0.x, 5.0.3 Jan 12, 2021
@Anipik Anipik merged commit e2fe57b into dotnet:release/5.0 Jan 14, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Feb 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants