Ensure LocalEndPoint being updated in SendToAsync(SocketAsyncEventArgs) by antonfirsov · Pull Request #808 · dotnet/runtime

antonfirsov · 2019-12-12T18:33:25Z

Fixes #915 by assigning _rightEndPoint like in other overloads, introducing a breaking change in the behavior of SendToAsync(SocketAsyncEventArgs).

Fixes #915

Fix #26882

dnfclas · 2019-12-12T18:33:39Z

All CLA requirements met.

antonfirsov · 2019-12-12T18:36:26Z

src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs

+            if (_rightEndPoint == null)
+            {
+                _rightEndPoint = endPointSnapshot;
+            }


I was not sure at which point should this happen. BeginSendTo assigns it before doing the actual WSA P/Invoke, I tried to follow that approach.

Nit: for future reference, this can now be written as:

_rightEndPoint ??= endPointSnapshot;

antonfirsov · 2019-12-12T18:36:59Z

src/libraries/System.Net.Sockets/tests/FunctionalTests/SocketTestHelper.cs

            Task.Run(() => s.Receive((Span<byte>)buffer, SocketFlags.None));
        public override Task<int> SendAsync(Socket s, ArraySegment<byte> buffer) =>
            Task.Run(() => s.Send((ReadOnlySpan<byte>)buffer, SocketFlags.None));
+        public override bool UsesSync => true;


I guess this was missing by mistake. The property seemed useful for my tests.

antonfirsov · 2019-12-12T21:30:30Z

@scalablecory @davidsh I just realized that my current changes are repeating the mistake pointed out in dotnet/corefx#40952. My understanding is that simply replacing the conditional block with Interlocked.CompareExchange would fix the race. Is this correct?

Can I address both issues in this PR, or shall I open a separate one?

scalablecory · 2019-12-12T23:02:19Z

@scalablecory @davidsh I just realized that my current changes are repeating the mistake pointed out in dotnet/corefx#40952. My understanding is that simply replacing the conditional block with Interlocked.CompareExchange would fix the race. Is this correct?

It's going to need more than a CAS. Here's what I'm seeing (each snip being a context switch)

user calls BeginRecieveFrom, and _rightEndPoint gets set.

runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs

Lines 3340 to 3343 in eae85a0

    
           if (_rightEndPoint == null) 
        
           { 
        
               _rightEndPoint = endPointSnapshot; 
        
           }

user calls BeginSendTo and _rightEndPoint is already set, so it is left alone.

runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs

Lines 2718 to 2721 in eae85a0

    
           if (_rightEndPoint == null) 
        
           { 
        
               _rightEndPoint = endPointSnapshot; 
        
           }

BeginReceiveFrom now fails and unsets _rightEndPoint

runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs

Lines 3349 to 3353 in eae85a0

    
           catch (ObjectDisposedException) 
        
           { 
        
               _rightEndPoint = oldEndPoint; 
        
               throw; 
        
           }

BeginSendTo succeeds, and _rightEndPoint is null, so LocalEndPoint will return null despite the socket now being implicitly bound.

Can I address both issues in this PR, or shall I open a separate one?

I think that's fine.

antonfirsov · 2019-12-17T15:13:19Z

/azp run runtime-libraries outerloop

azure-pipelines · 2019-12-17T15:13:34Z

Azure Pipelines successfully started running 1 pipeline(s).

antonfirsov · 2019-12-17T15:46:08Z

@dotnet/ncl after a further analysis testing of the code, my opinion got even stronger that we should handle the two topics separately. After we decide exactly how we change the way _rightEndpoint is being handled, we can change all it's usages in a separate PR.

I removed the no merge label, and marking this PR ready to review as is.

antonfirsov · 2019-12-17T18:57:45Z

A WebSocket and an Http outerloop test failed on linux.
I'm not familiar with those codebases yet, but I think it's very unlikely that they utilize SendToAsync(SocketAsyncEventArgs).

scalablecory

The other methods null out _rightEndPoint if the API call fails. We should be doing this here too, in the catch block below your change.

wfurt · 2019-12-18T23:53:02Z

fixes #915

antonfirsov · 2019-12-19T01:13:26Z

src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs

            catch
            {
+                _rightEndPoint = null;
                // Clear in-use flag on event args object.
                e.Complete();
                throw;
            }

+            if (!CheckErrorAndUpdateStatus(socketError))
+            {


I have no idea what black magic could trigger the catch block here, on the other hand socketError == AccessDenied is something we can actually hit, see my test cases. Like in DoBeginSendTo I'm resetting _rightEndPoint when hitting a non-success & non-pending case.

antonfirsov · 2019-12-19T01:19:34Z

@scalablecory just addressed your findings + did some test improvements:

Moved the new SendTo tests to it's own file & test classes
Added missing argument validation tests for all SendTo variants

scalablecory

lgtm

scalablecory · 2020-01-05T15:46:59Z

src/libraries/System.Net.Sockets/tests/FunctionalTests/SendReceive.cs

nit: extra line.

antonfirsov · 2020-01-08T13:37:51Z

Test failures are tracked under #1332 and unrelated to Sockets.

danmoseley · 2020-07-24T21:45:06Z

@antonfirsov is this missing a breaking change doc issue? If so can you please open one with https://github.com/dotnet/docs/issues/new?template=dotnet-breaking-change.md

Anton Firszov added 3 commits December 12, 2019 16:43

tests for implicit binding of LocalEndpoint

e6782e4

assign _rightEndPoint in SendToAsync(e)

8bccc77

Fix #26882

validate LocalEndPoint asynchronously in tests

f4782f7

Dotnet-GitSync-Bot added the area-System.Net.Sockets label Dec 12, 2019

antonfirsov commented Dec 12, 2019

View reviewed changes

davidsh added the breaking-change Issue or PR that represents a breaking API or functional change over a previous release. label Dec 12, 2019

davidsh added this to the 5.0 milestone Dec 12, 2019

antonfirsov commented Dec 12, 2019

View reviewed changes

antonfirsov requested a review from a team December 12, 2019 18:39

Anton Firszov added 2 commits December 12, 2019 19:48

remove unnecessary condition check in tests

dc5390c

missing line break

e01c3d3

antonfirsov added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Dec 12, 2019

Merge branch 'master' into fix-26882

2a42bd8

antonfirsov removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Dec 17, 2019

scalablecory reviewed Dec 18, 2019

View reviewed changes

Anton Firszov added 2 commits December 18, 2019 23:51

Merge remote-tracking branch 'upstream/master' into fix-26882

5c81405

add failing test, refactor tests

9547eee

Anton Firszov added 2 commits December 19, 2019 01:34

add argument validation tests + clean up test code

9423a70

address review finding (reset _rightEndPoint)

e009577

antonfirsov commented Dec 19, 2019

View reviewed changes

scalablecory approved these changes Jan 5, 2020

View reviewed changes

src/libraries/System.Net.Sockets/tests/FunctionalTests/SendReceive.cs Outdated

}

}

Copy link

Contributor

scalablecory Jan 5, 2020

Choose a reason for hiding this comment

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

nit: extra line.

remove extra newline

af52189

antonfirsov mentioned this pull request Jan 8, 2020

BasicTest_AccessInstanceProperties_NoExceptions_Bsd test failure #1332

Closed

antonfirsov merged commit a42d3a3 into master Jan 8, 2020

antonfirsov deleted the fix-26882 branch January 8, 2020 13:47

danmoseley added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Jul 30, 2020

antonfirsov mentioned this pull request Oct 6, 2020

Breaking change: Socket.LocalEndPoint updated on SendToAsync(SocketAsyncEventArgs e) dotnet/docs#20949

Closed

22 tasks

antonfirsov removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Oct 6, 2020

ghost locked as resolved and limited conversation to collaborators Dec 11, 2020

Conversation

antonfirsov commented Dec 12, 2019 • edited by karelz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnfclas commented Dec 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

antonfirsov Dec 12, 2019

Choose a reason for hiding this comment

Uh oh!

stephentoub Jan 8, 2020

Choose a reason for hiding this comment

Uh oh!

antonfirsov Dec 12, 2019

Choose a reason for hiding this comment

Uh oh!

antonfirsov commented Dec 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scalablecory commented Dec 12, 2019

Uh oh!

antonfirsov commented Dec 17, 2019

Uh oh!

azure-pipelines bot commented Dec 17, 2019

Uh oh!

antonfirsov commented Dec 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

antonfirsov commented Dec 17, 2019

Uh oh!

scalablecory left a comment

Choose a reason for hiding this comment

Uh oh!

wfurt commented Dec 18, 2019

Uh oh!

antonfirsov Dec 19, 2019

Choose a reason for hiding this comment

Uh oh!

antonfirsov commented Dec 19, 2019

Uh oh!

scalablecory left a comment

Choose a reason for hiding this comment

Uh oh!

scalablecory Jan 5, 2020

Choose a reason for hiding this comment

Uh oh!

antonfirsov commented Jan 8, 2020

Uh oh!

danmoseley commented Jul 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

antonfirsov commented Dec 12, 2019 •

edited by karelz

Loading

dnfclas commented Dec 12, 2019 •

edited

Loading

antonfirsov commented Dec 12, 2019 •

edited

Loading

antonfirsov commented Dec 17, 2019 •

edited

Loading