adding span version of ReceiveMessageFrom #46285

ovebastiansen · 2020-12-21T10:01:35Z

I followed the other examples of overloads with span and it seems like the normal is duplication of code, since the other methods are not that long. Should I keep it like this or call the span version from the byte[] version, and remove some of the duplication?

fix #43933

Dotnet-GitSync-Bot · 2020-12-21T10:01:38Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2020-12-21T10:01:42Z

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

Issue Details

I followed the other examples of overloads with span and it seems like the normal is duplication of code, since the other methods are not that long. Should I keep it like this or call the span version from the byte[] version, and remove some of the duplication?

fix #43933

Author:	ovebastiansen
Assignees:	-
Labels:	`area-System.Net.Sockets`, `new-api-needs-documentation`
Milestone:	-

wfurt · 2020-12-24T00:22:19Z

thanks for the contribution @ovebastiansen. It seems like you simply duplicated all the code we use with byte[] for Span<byte>. I think it would be better to spanify the impementaiton and update existing code to covert byte[] buffer, int offset, int size to Span and use the new implementation. The cost is low and I think we could avoid unnecessary code duplication.

ovebastiansen · 2020-12-24T08:32:07Z

Yes, I looked on some of the other span versions, and they seemed like duplications. Also asked this question in the opening. If I shouldn’t duplicate because of performance I can make it so that byte version calls the span version. //Ove On my phone

…

________________________________ From: Tomas Weinfurt <notifications@github.com> Sent: Thursday, December 24, 2020 1:22:31 AM To: dotnet/runtime <runtime@noreply.github.com> Cc: Ove Bastiansen <ovebastiansen@outlook.com>; Mention <mention@noreply.github.com> Subject: Re: [dotnet/runtime] adding span version of ReceiveMessageFrom (#46285) thanks for the contribution @ovebastiansen<https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fovebastiansen&data=04%7C01%7C%7C823f47605b8246b0c8f608d8a7a20184%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637443661538753203%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=6OcE7VHVGrkJNUIpcMAQQohK6aOCDOonWB99Jpc%2Bk2k%3D&reserved=0>. It seems like you simply duplicated all the code we use with byte[] for Span<byte>. I think it would be better to spanify the impementaiton and update existing code to covert byte[] buffer, int offset, int size to Span and use the new implementation. The cost is low and I think we could avoid unnecessary code duplication. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fdotnet%2Fruntime%2Fpull%2F46285%23issuecomment-750627076&data=04%7C01%7C%7C823f47605b8246b0c8f608d8a7a20184%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637443661538763195%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=KRXvQVD7ug61rGYBIjaXt7pJEFmJInxp0ToD2fN7%2BfA%3D&reserved=0>, or unsubscribe<https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAECK4AIQ63IEIQEATNDMFNLSWKCUPANCNFSM4VD7V6VQ&data=04%7C01%7C%7C823f47605b8246b0c8f608d8a7a20184%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637443661538763195%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=J0kID0ZDahialR6mK25mwtM9HEci3u8YW72M5XYbZBU%3D&reserved=0>.

ovebastiansen · 2020-12-28T16:30:08Z

/azp run runtime

azure-pipelines · 2020-12-28T16:30:15Z

Commenter does not have sufficient privileges for PR 46285 in repo dotnet/runtime

geoffkizer · 2021-01-05T19:49:41Z

It's kind of weird to add this just for ReceiveMessageFrom, but not ReceiveFrom or SendTo. I don't think we'd want to ship 6.0 like that.

ovebastiansen · 2021-01-05T19:52:04Z

I can fix those two as well then in this PR. Does the code otherwise look ok now?

geoffkizer · 2021-01-05T19:55:11Z

The reason we have duplication in a lot of these cases is because the sync implementation in SocketAsyncContext ends up treating Span<byte> differently than byte[] -- it can't store the Span<byte> on the heap, so it pins it and treats it as a native pointer. This isn't ideal, because it means the buffer is pinned for the duration of the call, as opposed to just when we actually call into the kernel.

In theory the code shouldn't need to do this pinning, but it that's a separate issue and isn't trivial to fix.

Addign @stephentoub in case he has any thoughts to add.

ovebastiansen · 2021-01-05T19:57:54Z

does that mean I should duplicate or keep it like this?

geoffkizer · 2021-01-05T20:06:46Z

Unfortunately, I think it means we should duplicate the code in the same way we do for Receive today. That is, the byte[] overload should use byte[] all the way down and not pin, and the Span overload should use Span all the way down, and pin when it creates the Operation object.

It would be good to fix this in the future; I'll file a separate issue on this.

geoffkizer · 2021-01-05T20:07:24Z

We need tests for these overloads too.

Adding @antonfirsov who has looked at these tests most recently. Anton, what's the best way to test these new overloads?

ovebastiansen · 2021-01-05T20:14:08Z

Ok, then I will go back to the first implementation, with the duplication. And I will also try to fix the other two methods you mentioned. I just figured they where tracked somewhere else in the sockets mega issue ;)

geoffkizer · 2021-01-05T20:15:42Z

Yeah, they are all tracked in #43935

geoffkizer · 2021-01-05T20:57:21Z

I added an issue re Span and pinning and code duplication, here: #46600

geoffkizer · 2021-01-12T20:13:03Z

#46862 from @antonfirsov should make testing this easy, we just need to ensure we have the right hooks enabled for the new overloads.

antonfirsov · 2021-01-13T10:51:15Z

Sorry, despite the tagging I somehow missed this PR 😞 I also didn't think through the fact that there is no span overload for ReceiveMessageFrom while working on #46862, and added a "span" version of the tests in autopilot mode:

runtime/src/libraries/System.Net.Sockets/tests/FunctionalTests/ReceiveMessageFrom.cs

Lines 156 to 159 in e464963

    
           public sealed class ReceiveMessageFrom_SpanSync : ReceiveMessageFrom<SocketHelperSpanSync> 
        
           { 
        
               public ReceiveMessageFrom_SpanSync(ITestOutputHelper output) : base(output) { } 
        
           }

On master, that test is now duplicate of the array test. In order to make pick up the span overload, SocketHelperSpanSync should be extended with an override of ReceiveMessageFromAsync:

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

Lines 413 to 421 in e464963

    
           public class SocketHelperSpanSync : SocketHelperArraySync 
        
           { 
        
               public override bool ValidatesArrayArguments => false; 
        
               public override Task<int> ReceiveAsync(Socket s, ArraySegment<byte> buffer) => 
        
                   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; 
        
           }

This should be sufficient to provide the necessary test coverage for this PR.

antonfirsov

Should be good after some cleanup, updating the tests and removing the code duplication in the Windows PAL.

I have a few (optional) suggestions though.

antonfirsov · 2021-01-13T10:58:28Z

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

+
+namespace System.Net.Sockets.Tests
+{
+    public class ReceiveMessageFromSpan


The whole class can be deleted now in favor of the SocketTestHelperSpan extension suggested in #46285 (comment).

However, in order to stress slicing, I would alter ReceiveSentMessages_Success to use an ArraySegment with a non-zero offset as a receiveBuffer:

runtime/src/libraries/System.Net.Sockets/tests/FunctionalTests/ReceiveMessageFrom.cs

Line 36 in 699f49a

byte[] receiveBuffer = new byte[DatagramSize];

I tried adding a bigger array receive array and offset it. Mostly seems to be working, checking that the part before the offset is not written to, and that the rest is the same. But now the EAP version fails, somewhere in the code it does not respect the offset it seems, and just tries to write to the start of the array. I have looked at the entrypart of the code and cannot spot an easy reason for this. Can you check it, @antonfirsov?

@antonfirsov Where you able to spot any problems in this? EAP tests fails when running with an offset

@ovebastiansen I'm sorry, missed your original comment, looks like a bug, opened an issue: #47637.

Please modify the beginning of your test as following, so you are not blocked by this:

// [ActiveIssue("https://github.com/dotnet/runtime/issues/47637")] int offset = UsesSync ? 10 : 0;

Edit: There was a typo in the original suggestion. Should be UsesSync ? 10 : 0

Even better if you do it like: UsesSync || !PlatformDetection.IsWindows ? 10 : 0

src/libraries/System.Net.Sockets/tests/FunctionalTests/System.Net.Sockets.Tests.csproj

src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Windows.cs

src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncContext.Unix.cs

antonfirsov · 2021-01-13T12:32:08Z

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

@@ -1564,6 +1564,73 @@ public int ReceiveMessageFrom(byte[] buffer, int offset, int size, ref SocketFla
            return bytesTransferred;
        }

+        public int ReceiveMessageFrom(Span<byte> buffer, ref SocketFlags socketFlags, ref EndPoint remoteEP, out IPPacketInformation ipPacketInformation)


The new practice is to add xmldocs right in the dotnet/runtime PR. See 9f73188 from #45083

Hmm, it seems like non of the other methods in this class has it, should I start the it or am I looking at the wrong place?

You can check #47230 and #47229 as examples adding new methods to Socket.cs together with xml docs.

Just copy existing docs, and modify them according to the new overload:
https://docs.microsoft.com/en-us/dotnet/api/system.net.sockets.socket.receivemessagefrom?view=net-5.0

took a stab at the documentation, copied from the existing, but existing is missing some of the exception actually thrown from the method, so a bit unsure if they should be added.

antonfirsov

I think we are good to merge after fixing the merge conflicts and adding the xml docs.

The conflicts are coming from recent PR-s around the area, and I don't expect them to be hard to fix.

antonfirsov · 2021-01-29T17:53:19Z

@ovebastiansen I opened #47642 to fix the async Windows bug, if it gets merged before your PR, you don't need to bother with the [ActiveIssue] stuff.

fix dotnet#43933

antonfirsov · 2021-01-31T10:21:39Z

/azp run runtime-libraries-coreclr outerloop

azure-pipelines · 2021-01-31T10:21:51Z

Azure Pipelines successfully started running 1 pipeline(s).

antonfirsov

LGTM after fixing two nits.

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

src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncContext.Unix.cs

antonfirsov · 2021-02-01T12:18:48Z

Outer loop test failures are unrelated: #41606, #43207, #46619, #40798, #43877, #44352

antonfirsov · 2021-02-02T09:36:14Z

Test failure is in the latest (inner) run is #47728.

antonfirsov

I just realized we still need to fix stuff in xmldocs, for example adding a copy of the long remarks section of the array overload. Regardless, I want to merge this as-is to unblock follow-up work, and will rather open a separate PR with the docs myself instead of delaying this PR further. new-api-needs-documentation PR-s contain an unfair amount of formal expectations towards community contributors IMO. @carlossanlop thoughts on this?

@ovebastiansen thanks for the work and the patience!

Dotnet-GitSync-Bot added area-System.Net.Sockets new-api-needs-documentation labels Dec 21, 2020

runfoapp bot mentioned this pull request Dec 21, 2020

SelfContained_NetCoreApp3_CompatMode_SingleFile_APITests failed in CI #43917

Closed

ovebastiansen force-pushed the issue-43933 branch from 6637616 to c5df76f Compare December 22, 2020 09:59

ovebastiansen force-pushed the issue-43933 branch from c5df76f to c13e3fe Compare December 26, 2020 15:14

ovebastiansen force-pushed the issue-43933 branch from 47ba28d to a3961ab Compare December 28, 2020 17:02

ovebastiansen force-pushed the issue-43933 branch from a3961ab to f2c2a3d Compare January 6, 2021 19:18

antonfirsov self-assigned this Jan 13, 2021

antonfirsov reviewed Jan 13, 2021

View reviewed changes

ovebastiansen force-pushed the issue-43933 branch from f2c2a3d to 2666248 Compare January 23, 2021 10:59

antonfirsov reviewed Jan 29, 2021

View reviewed changes

antonfirsov mentioned this pull request Jan 29, 2021

Socket.ReceiveMessageFrom(saea) does not respect buffer offset on Windows #47637

Closed

ovebastiansen added 5 commits January 29, 2021 21:17

adding span version of ReceiveMessageFrom

24d5af0

fix dotnet#43933

Started using new testsetup for the receivemessagefrom

8facd83

removed code duplication in socketpal.windows

ac76468

removed unused buffers parameter

2efca1e

adding non-zero offset to the ReceiveMessageFrom test

57c84d3

ovebastiansen force-pushed the issue-43933 branch from 7bd0275 to 57c84d3 Compare January 29, 2021 20:20

ovebastiansen added 2 commits January 30, 2021 17:08

added documentation

e106a97

offset fix for EAP tests

5e7a035

ovebastiansen requested a review from antonfirsov January 30, 2021 18:35

antonfirsov approved these changes Feb 1, 2021

View reviewed changes

src/libraries/System.Net.Sockets/ref/System.Net.Sockets.cs Outdated Show resolved Hide resolved

src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncContext.Unix.cs Outdated Show resolved Hide resolved

antonfirsov mentioned this pull request Feb 1, 2021

SocketAsyncContext should not need to pin Span buffers for sync APIs #46600

Open

fixing pr comments

871f535

antonfirsov approved these changes Feb 2, 2021

View reviewed changes

antonfirsov merged commit 4df29c9 into dotnet:master Feb 2, 2021

runfoapp bot mentioned this pull request Feb 2, 2021

HttpWebRequestTest_Sync.ReadWriteTimeout_CancelsResponse failed in CI mono Linux x64 #47728

Closed

ghost locked as resolved and limited conversation to collaborators Mar 4, 2021

karelz added this to the 6.0.0 milestone May 20, 2021

adding span version of ReceiveMessageFrom #46285

adding span version of ReceiveMessageFrom #46285

Conversation

ovebastiansen commented Dec 21, 2020

Dotnet-GitSync-Bot commented Dec 21, 2020

ghost commented Dec 21, 2020

wfurt commented Dec 24, 2020

ovebastiansen commented Dec 24, 2020 via email

ovebastiansen commented Dec 28, 2020

azure-pipelines bot commented Dec 28, 2020

geoffkizer commented Jan 5, 2021

ovebastiansen commented Jan 5, 2021

geoffkizer commented Jan 5, 2021

ovebastiansen commented Jan 5, 2021

geoffkizer commented Jan 5, 2021

geoffkizer commented Jan 5, 2021

ovebastiansen commented Jan 5, 2021

geoffkizer commented Jan 5, 2021

geoffkizer commented Jan 5, 2021

geoffkizer commented Jan 12, 2021

antonfirsov commented Jan 13, 2021 • edited Loading

antonfirsov left a comment • edited Loading

Choose a reason for hiding this comment

antonfirsov Jan 13, 2021 • edited Loading

Choose a reason for hiding this comment

ovebastiansen Jan 23, 2021

Choose a reason for hiding this comment

ovebastiansen Jan 29, 2021

Choose a reason for hiding this comment

antonfirsov Jan 29, 2021 • edited Loading

Choose a reason for hiding this comment

antonfirsov Jan 29, 2021

Choose a reason for hiding this comment

antonfirsov Jan 13, 2021

Choose a reason for hiding this comment

ovebastiansen Jan 23, 2021

Choose a reason for hiding this comment

antonfirsov Jan 29, 2021

Choose a reason for hiding this comment

ovebastiansen Jan 30, 2021

Choose a reason for hiding this comment

antonfirsov left a comment • edited Loading

Choose a reason for hiding this comment

antonfirsov commented Jan 29, 2021

antonfirsov commented Jan 31, 2021

azure-pipelines bot commented Jan 31, 2021

antonfirsov left a comment

Choose a reason for hiding this comment

antonfirsov commented Feb 1, 2021

antonfirsov commented Feb 2, 2021

antonfirsov left a comment • edited Loading

Choose a reason for hiding this comment

antonfirsov commented Jan 13, 2021 •

edited

Loading

antonfirsov left a comment •

edited

Loading

antonfirsov Jan 13, 2021 •

edited

Loading

antonfirsov Jan 29, 2021 •

edited

Loading

antonfirsov left a comment •

edited

Loading

antonfirsov left a comment •

edited

Loading