Add Span-based overloads to System.Net.Sockets#22988

stephentoub · 2017-08-06T03:07:17Z

Span-based synchronous overloads of:

Socket.Receive/Send
NetworkStream.Read.Write

Contributes to https://github.com/dotnet/corefx/issues/22608
Contributes to https://github.com/dotnet/corefx/issues/22387
cc: @geoffkizer, @CIPop, @davidsh, @KrzysztofCwalina

(Buffer-based async overloads will be done later once Buffer<T> is available.)

justinvp · 2017-08-06T04:35:45Z

src/System.Net.Sockets/src/System/Net/Sockets/NetworkStream.cs


+        public override int Read(Span<byte> destination)
+        {
+            if (_cleanedUp) throw new ObjectDisposedException(this.GetType().FullName);


Nit: Some of the new methods use this. while others do not.

Will fix. Thanks.

stephentoub · 2017-08-06T14:55:18Z

@mellinoe, another seg fault in the drawing tests, this time on centos:

2017-08-06 13:12:11,172: INFO: proc(54): run_and_log_output: Output: Discovering: System.Drawing.Common.Tests
2017-08-06 13:12:11,979: INFO: proc(54): run_and_log_output: Output: Discovered:  System.Drawing.Common.Tests
2017-08-06 13:12:12,498: INFO: proc(54): run_and_log_output: Output: Starting:    System.Drawing.Common.Tests
2017-08-06 13:12:13,469: INFO: proc(54): run_and_log_output: Output: /home/helixbot/dotnetbuild/work/684b6a91-2f4e-408e-9807-5d617f1bddb8/Work/c393caf7-083f-40c4-abdb-032acd67b1ea/Unzip/RunTests.sh: line 87: 65984 Segmentation fault      (core dumped) $RUNTIME_PATH/dotnet xunit.console.netcore.exe System.Drawing.Common.Tests.dll -xml testResults.xml -notrait Benchmark=true -notrait category=nonnetcoreapptests -notrait category=nonlinuxtests -notrait category=OuterLoop -notrait category=failing

https://ci3.dot.net/job/dotnet_corefx/job/master/job/linux-TGroup_netcoreapp+CGroup_Release+AGroup_x64+TestOuter_false_prtest/2046/

@dotnet-bot test Linux x64 Release Build please

davidsh

LGTM.

I just want to add that despite writing tests specifically for 'netcoreapp', these new APIs are part of .NET Core 2.1. And that means that they will also be exposed and work on UWP as well (post UWP6.0).

The current architecture of our ref folders means that any API surface that is part of CoreFx repo will be exposed both for 'netcoreapp' and 'uap'. Not a bad thing, per se. But that means we should be testing it. I would prefer in general that we include the extra tests (that have the .netcoreapp.cs suffix) in UAP tests as well. Thus, we will need a UAP test configuration in addition to the 'netstandard' and 'netcoreapp' configs.

cc: @weshaggard @danmosemsft

mellinoe · 2017-08-07T18:05:30Z

@stephentoub I can't find the segfault in the logs. Do the logs get wiped if you force a re-run?

stephentoub · 2017-08-07T19:36:47Z

I believe they unfortunately get overwritten. @MattGal? @mmitche?

mmitche · 2017-08-07T19:39:55Z

The jenkins logs don't (though you have to go back and find the previous run of that job), not sure about Helix.

MattGal · 2017-08-07T20:08:03Z

@mellinoe We don't expose it but logs are never actually overwritten. The log you're likely looking for is here.

It looks like there's a dumpling bug at play here, as a core dump was created but failed to upload too:

2017-08-06 13:12:43,505: INFO: proc(54): run_and_log_output: Output: processing dump file /home/helixbot/dotnetbuild/work/684b6a91-2f4e-408e-9807-5d617f1bddb8/Work/c393caf7-083f-40c4-abdb-032acd67b1ea/Unzip/core.65984
2017-08-06 13:12:43,506: INFO: proc(54): run_and_log_output: Output: creating dumpling dump 02af3a6a179a812c6fa26723b4d32954344bafc7
2017-08-06 13:12:43,506: INFO: proc(54): run_and_log_output: Output: uploading artifact 02af3a6a179a812c6fa26723b4d32954344bafc7 core.65984
2017-08-06 13:12:43,506: INFO: proc(54): run_and_log_output: Output: Traceback (most recent call last):
2017-08-06 13:12:43,506: INFO: proc(54): run_and_log_output: Output:   File "/home/helixbot/.dumpling/dumpling.py", line 1128, in <module>
2017-08-06 13:12:43,506: INFO: proc(54): run_and_log_output: Output:     main(sys.argv)
2017-08-06 13:12:43,506: INFO: proc(54): run_and_log_output: Output:   File "/home/helixbot/.dumpling/dumpling.py", line 1123, in main
2017-08-06 13:12:43,506: INFO: proc(54): run_and_log_output: Output:     cmdProc.Process(config)
2017-08-06 13:12:43,506: INFO: proc(54): run_and_log_output: Output:   File "/home/helixbot/.dumpling/dumpling.py", line 529, in Process
2017-08-06 13:12:43,506: INFO: proc(54): run_and_log_output: Output:     self.Upload(config)
2017-08-06 13:12:43,506: INFO: proc(54): run_and_log_output: Output:   File "/home/helixbot/.dumpling/dumpling.py", line 624, in Upload
2017-08-06 13:12:43,506: INFO: proc(54): run_and_log_output: Output:     self.UploadDump(config)
2017-08-06 13:12:43,506: INFO: proc(54): run_and_log_output: Output:   File "/home/helixbot/.dumpling/dumpling.py", line 644, in UploadDump
2017-08-06 13:12:43,506: INFO: proc(54): run_and_log_output: Output:     self._triage_dump(dumpid, config.dumppath, config)
2017-08-06 13:12:43,507: INFO: proc(54): run_and_log_output: Output:   File "/home/helixbot/.dumpling/dumpling.py", line 812, in _triage_dump
2017-08-06 13:12:43,507: INFO: proc(54): run_and_log_output: Output:     if config.dbgpath is None:

mellinoe · 2017-08-07T20:22:49Z

@MattGal Thanks for that link. The crash dump was uploaded, actually. I will see if it gives me any useful information.

geoffkizer · 2017-08-08T00:07:28Z

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

            {
-                private get { return (Action<int, byte[], int, SocketFlags, SocketError>)CallbackOrEvent; }
-                set { CallbackOrEvent = value; }
+                set => CallbackOrEvent = value;


This seems like a good change, but we should be consistent about doing it this way across all operations. I'm hacking on this code and can do this in a subsequent change.

geoffkizer · 2017-08-08T00:11:48Z

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

+                ((Action<int, byte[], int, SocketFlags, SocketError>)CallbackOrEvent)(BytesTransferred, SocketAddress, SocketAddressLen, SocketFlags.None, ErrorCode);
+        }
+
+        private sealed class BufferArraySendOperation : SendOperation


Do we really need this? Can't we just use BufferPtrSendOperation below?

We could, but then we'd need to pin the buffer for potentially much longer than we otherwise need to. The byte[] only needs to be pinned during the synchronous call to send, but to use the BufferPtr version, we need to pin it the whole time in order to get the pointer and keep it valid.

geoffkizer · 2017-08-08T00:12:57Z

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

+            protected override bool DoTryComplete(SocketAsyncContext context)
+            {
+                int bufferIndex = 0;
+                return SocketPal.TryCompleteSendTo(context._socket, new ReadOnlySpan<byte>(BufferPtr, Offset + Count), null, ref bufferIndex, ref Offset, ref Count, Flags, SocketAddress, SocketAddressLen, ref BytesTransferred, out ErrorCode);


Why Offset + Count? I would expect this to be new ReadOnlySpan(BufferPtr, Count) -- am I missing something?

Why Offset + Count?

The TryCompleteSendTo implementation uses the ref Offset, ref Count arguments to determine from where and how much to send, and then updates those values accordingly for a subsequent operation. This means the input span needs to always be one that points to the full original position and size. BufferPtr is the original starting position, and the original length is Offset + Count, even as Offset might be increased and Count might be decreased for partial sends.

Yeah, I recall now. The usage always confuses me, but it's a separate issue from this PR.

geoffkizer · 2017-08-08T00:18:56Z

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

+            }
+        }
+
+        public unsafe SocketError ReceiveFrom(Span<byte> buffer, ref SocketFlags flags, byte[] socketAddress, ref int socketAddressLen, int timeout, out int bytesReceived)


Do we need both this and the existing overload with (byte[], int offset, int count)? We can always go from byte[] -> Span, so why not just have this version and update the callers?

We can always go from byte[] to span, but we can't go back, and we can't store the span on the heap. Per your question earlier, if we're ok using the BufferPtr variant and pinning the buffer longer than is potentially otherwise needed, then we could consolidate these paths, otherwise I don't see how we can without incurring additional cost.

geoffkizer · 2017-08-08T00:31:06Z

My general concern here is that we're adding Span paths through the code while leaving existing paths with (byte[], int offset, int count) in place.

I assume the long-term plan is to use Span (or Buffer) everywhere internally and convert to Span/Buffer at the top edge, right?

I realize that cleaning all this up is probably not worthwhile in this PR, but in some cases (e.g. the additions to SocketAsyncContext) it seems worthwhile to do, rather than duplicating existing code.

stephentoub · 2017-08-08T02:05:27Z

My general concern here is that we're adding Span paths through the code while leaving existing paths with (byte[], int offset, int count) in place. I assume the long-term plan is to use Span (or Buffer) everywhere internally and convert to Span/Buffer at the top edge, right?

I've consolidated where I saw we could do so without incurring potentially non-trivial expense.

geoffkizer · 2017-08-08T02:10:52Z

Yeah, I understand now that there's a tradeoff re pinning that makes further consolidation questionable.

It seems like where we've landed here, the guidance we'd give to customers would be to avoid using Span when they can use (buffer, offset, count) instead. Basically, only use it for native or stackalloc memory.
Otherwise they are paying a price in terms of pinning behavior.

That seems unfortunate to me. Am I missing something?

stephentoub · 2017-08-08T02:15:37Z

YIt seems like where we've landed here, the guidance we'd give to customers would be to avoid using Span when they can use (buffer, offset, count) instead. Basically, only use it for native or stackalloc memory. Otherwise they are paying a price in terms of pinning behavior. That seems unfortunate to me. Am I missing something?

You mean specifically with the synchronous socket APIs (rather than span in general)? In reality I don't know if/how much the pinning behavior would actually hurt things; consider, for example, that if the synchronous native call would instead block for the duration of the operation (e.g. in blocking rather than non-blocking mode), the buffer is going to be pinned for that whole P/Invoke any way... so this is really no different than that. I'm just being conservative in how these are implemented in this first go around to avoid negatively impacting existing overloads.

geoffkizer · 2017-08-08T02:20:35Z

Okay, seems reasonable to me.

- Socket.Receive/Send - NetworkStream.Read.Write

* Add Span-based overloads to System.Net.Sockets - Socket.Receive/Send - NetworkStream.Read.Write * Address PR feedback Commit migrated from dotnet/corefx@4a97f92

stephentoub added the area-System.Net.Sockets label Aug 6, 2017

dnfclas added the cla-already-signed label Aug 6, 2017

justinvp reviewed Aug 6, 2017

View reviewed changes

karelz assigned stephentoub Aug 6, 2017

stephentoub force-pushed the sockets_span branch from 91a38e1 to 011b735 Compare August 6, 2017 12:47

davidsh approved these changes Aug 6, 2017

View reviewed changes

geoffkizer reviewed Aug 8, 2017

View reviewed changes

stephentoub added 2 commits August 7, 2017 22:55

Add Span-based overloads to System.Net.Sockets

79bbcd3

- Socket.Receive/Send - NetworkStream.Read.Write

Address PR feedback

5e6fa6f

stephentoub force-pushed the sockets_span branch from 011b735 to 5e6fa6f Compare August 8, 2017 03:03

stephentoub merged commit 4a97f92 into dotnet:master Aug 8, 2017

stephentoub deleted the sockets_span branch August 8, 2017 12:40

karelz modified the milestone: 2.1.0 Aug 14, 2017

stephentoub added the netfx-port-consider label Sep 15, 2017

ManickaP mentioned this pull request Jul 3, 2020

Offset edge case bug fix? dotnet/runtime#38762

Closed

Comments

Conversation

stephentoub commented Aug 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub commented Aug 6, 2017

Uh oh!

davidsh left a comment

Choose a reason for hiding this comment

Uh oh!

mellinoe commented Aug 7, 2017

Uh oh!

stephentoub commented Aug 7, 2017

Uh oh!

mmitche commented Aug 7, 2017

Uh oh!

MattGal commented Aug 7, 2017

Uh oh!

mellinoe commented Aug 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geoffkizer commented Aug 8, 2017

Uh oh!

stephentoub commented Aug 8, 2017

Uh oh!

geoffkizer commented Aug 8, 2017

Uh oh!

stephentoub commented Aug 8, 2017

Uh oh!

geoffkizer commented Aug 8, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants