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

Socket LocalEndPoint not updated in SendToAsync(SocketAsyncEventArgs e) #915

Closed
funkynicco opened this issue Feb 6, 2018 · 7 comments · Fixed by #808
Closed

Socket LocalEndPoint not updated in SendToAsync(SocketAsyncEventArgs e) #915

funkynicco opened this issue Feb 6, 2018 · 7 comments · Fixed by #808
Assignees
Labels
area-System.Net.Sockets breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. bug
Milestone

Comments

@funkynicco
Copy link

The SocketAsyncEventArgs version of SendToAsync does not properly set _rightEndPoint, while it creates the endPointSnapshot variable that is used to set _rightEndPoint in SendTo, BeginSendTo and SendToAsync (task) methods.

According to remarks section of the documentation on recvfrom method, all of sendto, WSASendTo and WSAJoinLeaf implicitly binds to a local address which is reflected by calling getsockname.

Here is a reference link for Socket.cs with line number.
https://github.com/dotnet/corefx/blob/master/src/System.Net.Sockets/src/System/Net/Sockets/Socket.cs#L4213
Otherwise it is in public bool SendToAsync(SocketAsyncEventArgs e) method.

Example code for reproducing issue

static void Main(string[] args)
{
    using (var socket = new Socket(AddressFamily.InterNetwork, SocketType.Dgram, ProtocolType.Udp))
    {
        var destinationEndPoint = new IPEndPoint(IPAddress.Parse("10.20.30.40"), 1234);

        var socketAsyncEventArgs = new SocketAsyncEventArgs();
        socketAsyncEventArgs.RemoteEndPoint = destinationEndPoint;
        socketAsyncEventArgs.SetBuffer(new byte[32], 0, 32); // 32 bytes of zeros
        socketAsyncEventArgs.Completed += (sender, e) => SendCompleted(e);

        if (!socket.SendToAsync(socketAsyncEventArgs))
            SendCompleted(socketAsyncEventArgs);

        Debug.Assert(socket.LocalEndPoint != null); // LocalEndPoint should not be null
    }

    Console.ReadLine();
}

static void SendCompleted(SocketAsyncEventArgs args)
{
    args.Dispose();
}
@karelz
Copy link
Member

karelz commented Feb 6, 2018

Seems like a bug to me (disclaimer: I don't have much expertise in that code, so I may be wrong).
Are you interested in submitting a PR?

@funkynicco
Copy link
Author

I'm unfortunately not an expert in this source either, I'd prefer that a more experienced person have a look over this to make sure the right solution is provided considering this is very low core level code that may potentially break several server implementations and frameworks.

@karelz
Copy link
Member

karelz commented Sep 10, 2019

Triage: We should check .NET Framework behavior. How does it behave on Linux?
We should make our APIs to be consistent a bit.

@antonfirsov
Copy link
Member

I can confirm, that this behavior is also reproducible on Linux and .NET Framework. If we fix the issue, we break consistency.
@karelz @stephentoub @davidsh can any of you give a suggestion how to proceed?

@davidsh
Copy link
Contributor

davidsh commented Dec 6, 2019

@karelz @stephentoub @davidsh can any of you give a suggestion how to proceed?

Can we get the correct (i.e. fixed) behavior consistent across .NET Core (Windows/Linux/Mac)? That would be the priority. It's probably ok if we have dissimilar behavior on .NET Framework. We can document that difference on docs.microsoft.com.

@antonfirsov
Copy link
Member

@davidsh the code responsible for this doesn't seem platform specific to me, so it is easy to keep it consistent for .NET Core.

Note: socket.RemoteEndpoint is also left null when calling the SocketAsyncEventArgs overloads, while the others are altering it. It seems logical to change the behavior for both properties.

I wonder if there any guidelines (or at least precedents) for introducing behavioral deviations between .NET Core and .NET Framework?

@davidsh
Copy link
Contributor

davidsh commented Dec 6, 2019

I wonder if there any guidelines (or at least precedents) for introducing behavioral deviations between .NET Core and .NET Framework?

We already have places in the networking code where we have differences between .NET Core and .NET Framework. And we document them on docs.microsoft.com usually in the 'Remarks' section of our APIs.

See example: https://docs.microsoft.com/en-us/dotnet/api/system.net.http.httprequestmessage.version?view=netcore-3.0#System_Net_Http_HttpRequestMessage_Version

If you plan to submit a PR for this, we'll want to mark it as potential 'Breaking Change' to remind us to document it for .NET 5. We have an existing label for that:
image

@antonfirsov antonfirsov self-assigned this Dec 11, 2019
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Net.Sockets untriaged New issue has not been triaged by the area owner labels Dec 16, 2019
@maryamariyan maryamariyan transferred this issue from dotnet/corefx Dec 16, 2019
@maryamariyan maryamariyan added this to the Future milestone Dec 16, 2019
@maryamariyan maryamariyan added this to To do in Networking ramp-up via automation Dec 16, 2019
@maryamariyan maryamariyan moved this from To do to In progress in Networking ramp-up Dec 16, 2019
@karelz karelz added breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. and removed untriaged New issue has not been triaged by the area owner labels Jan 7, 2020
Networking ramp-up automation moved this from In progress to Done Jan 8, 2020
antonfirsov added a commit that referenced this issue Jan 8, 2020
…s) (#808)

Fixes #915 by assigning _rightEndPoint like in other overloads, introducing a breaking change in the behavior of SendToAsync(SocketAsyncEventArgs).
@karelz karelz modified the milestones: Future, 5.0.0 Aug 18, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Sockets breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. bug
Projects
Development

Successfully merging a pull request may close this issue.

6 participants