UdpClient with span support #53429

lateapexearlyspeed · 2021-05-28T15:47:25Z

Fix #864 , I am starting working on it.

dotnet-issue-labeler · 2021-05-28T15:47:30Z

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 · 2021-05-28T15:47:33Z

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

Issue Details

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

antonfirsov · 2021-05-28T16:12:33Z

@lateapexearlyspeed thanks, let us know if you have any questions in the area.

geoffkizer · 2021-05-28T17:32:13Z

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

@@ -757,6 +757,7 @@ public partial class UdpClient : System.IDisposable
        public byte[] Receive([System.Diagnostics.CodeAnalysis.NotNullAttribute] ref System.Net.IPEndPoint? remoteEP) { throw null; }
        public System.Threading.Tasks.Task<System.Net.Sockets.UdpReceiveResult> ReceiveAsync() { throw null; }
        public int Send(byte[] dgram, int bytes) { throw null; }
+        public int Send(ReadOnlySpan<byte> dgram) {throw null; }


The param name as approved by API review is "datagram".

Fixed, thanks.

…ostname, port) overloads.

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

geoffkizer · 2021-06-01T18:16:34Z

See my comment on the API as approved here: #864 (comment)

lateapexearlyspeed · 2021-06-02T02:31:26Z

Hi @geoffkizer Just confirm, based on comment, you have decided to "match existing API nullability and semantics when null is used" now. So Happily current code in PR just meets this :)

geoffkizer · 2021-06-02T19:36:14Z

So Happily current code in PR just meets this :)

Yes :)

…ostname, int port ...); Add test.

lateapexearlyspeed · 2021-06-07T09:30:12Z

Hi @geoffkizer , this PR is ready for review now, thanks.

geoffkizer · 2021-06-08T05:07:39Z

@antonfirsov Can you review?

antonfirsov

Thanks for the addition @lateapexearlyspeed! The change is conceptually fine, but we need to fix a couple of small things before merging.

antonfirsov · 2021-06-08T09:50:46Z

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

@@ -600,9 +612,15 @@ public void DropMulticastGroup(IPAddress multicastAddr, int ifindex)
        public Task<int> SendAsync(byte[] datagram, int bytes) =>
            SendAsync(datagram, bytes, null);

+        public ValueTask<int> SendAsync(ReadOnlyMemory<byte> datagram, CancellationToken cancellationToken = default) =>


According to a new rule, we need to add triple-slash docs for all new public methods right in product code PR-s. They will be fed to a tool to generate API docs.

You can find examples in Socket.cs. My recommendation is to copy and alter documentation text from existing overloads

Added xml doc for new APIs.

antonfirsov · 2021-06-08T09:54:16Z

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

+            Assert.Throws<ObjectDisposedException>(() => udpClient.Send(new ReadOnlySpan<byte>()));
+            Assert.Throws<ObjectDisposedException>(() => udpClient.Send(new ReadOnlySpan<byte>(), "localhost", 0));
+
+            await Assert.ThrowsAsync<ObjectDisposedException>(() => udpClient.SendAsync(new ReadOnlyMemory<byte>(), remoteEP).AsTask());


Suggested change

await Assert.ThrowsAsync<ObjectDisposedException>(() => udpClient.SendAsync(new ReadOnlyMemory<byte>(), remoteEP).AsTask());

Assert.Throws<ObjectDisposedException>(() => _ = udpClient.SendAsync(new ReadOnlyMemory<byte>(), remoteEP));

The ObjectDisposedException-s should be thrown synchronously (same for the rest).

Let's also add coverage for the existing SendAsync overloads that are not covered here.

Fixed and added.

antonfirsov · 2021-06-08T10:08:04Z

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

+        [OuterLoop]
+        [Theory]
+        [MemberData(nameof(Loopbacks))]
+        public async Task SendToRecvFromAsyncWithReadOnlyMemory_Datagram_UDP_UdpClient(IPAddress loopbackAddress)


Instead of duplicating the whole test case, can we add an additional bool useMemoryOverload parameter to the previous one?

antonfirsov · 2021-06-08T10:56:54Z

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

        public Task<int> SendAsync(byte[] datagram, int bytes, string? hostname, int port) =>
            SendAsync(datagram, bytes, GetEndpoint(hostname, port));

+        public ValueTask<int> SendAsync(ReadOnlyMemory<byte> datagram, string? hostname, int port, CancellationToken cancellationToken = default) =>
+            SendAsync(datagram, GetEndpoint(hostname, port), cancellationToken);


Since this is an async overload, we should not use the blocking Dns.GetHostAddresses when resolving hostname. We need an async variant of GetEndpoint.

I agree we should have an async version of GetEndpoint. That said, the existing SendAsync above just uses sync GetEndpoint. So since we don't have this today, I think we could live without it for this PR and file a separate issue.

antonfirsov · 2021-06-08T11:01:59Z

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

@@ -334,6 +335,17 @@ private void ValidateDatagram(byte[] datagram, int bytes, IPEndPoint? endPoint)
            }
        }

+        private void ValidateDatagram(ReadOnlyMemory<byte> datagram, IPEndPoint? endPoint)


The datagram argument is unused, note that this also makes the naming unfortunate. Since this particular overload has only one usage, it's cleaner to inline probably.

Refactored code to validate inside calling code.

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

antonfirsov · 2021-06-08T11:52:18Z

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

@@ -606,6 +621,40 @@ public async Task SendAsync_ReceiveAsync_Success(bool ipv4)
            }
        }

+        [OuterLoop]


Let's remove [OuterLoop] from all send/receive tests in UdpClientTest. It's a leftover from dotnet/corefx@defbb8a which was a carpet bombing.

antonfirsov · 2021-06-08T11:54:55Z

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

+            using (var receiver = new UdpClient(new IPEndPoint(address, 0)))
+            using (var sender = new UdpClient(new IPEndPoint(address, 0)))
+            {
+                await sender.SendAsync(new ReadOnlyMemory<byte>(new byte[1]), new IPEndPoint(address, ((IPEndPoint)receiver.Client.LocalEndPoint).Port));


This only tests one overload, we need to also test the remaining ones (especially the ones using GetEndpoint).

I know this is a debt that also applies to other overloads, but we need to deal with it before proceeding, and make sure every new API we add is tested.

antonfirsov · 2021-06-08T11:55:31Z

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

+        }
+
+        [Fact]
+        [PlatformSpecific(TestPlatforms.Windows)] // "localhost" resolves to IPv4 & IPV6 on Windows, but may resolve to only one of those on Unix


Rather than making this test platform specific, can we test cancellation with an unconnected UdpClient?

antonfirsov · 2021-06-08T12:00:53Z

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

+            using (var receiver = new UdpClient("localhost", 0))
+            using (var sender = new UdpClient("localhost", ((IPEndPoint)receiver.Client.LocalEndPoint).Port))
+            {
+                await sender.SendAsync(new ReadOnlyMemory<byte>(new byte[1]));


Same as in SendAsyncWithReadOnlyMemory_ReceiveAsync_Success: we need to test the other overloads too. We also need to verify if InvalidOperationException is being thrown on hostName != null or port != 0 for a connected UdpClient.

lateapexearlyspeed · 2021-06-15T10:41:14Z

There is one thing not sure:
the existing method public Task SendAsync(byte[] datagram, int bytes) did not check if current udpClient is not connected but existing sync version checked. The new async version keep same as public Task<int> SendAsync(byte[] datagram, int bytes).
Please clarify if we should fix existing behavior.

antonfirsov

@lateapexearlyspeed sorry for the delay, and thanks a lot for your patience and accurate work! Just a few more requests and we are done.

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

antonfirsov · 2021-06-17T12:14:11Z

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

+        /// <exception cref="SocketException">An error occurred when accessing the socket.</exception>
+        public ValueTask<int> SendAsync(ReadOnlyMemory<byte> datagram, CancellationToken cancellationToken = default) =>


This is makes the bar really unfair for community contributors IMO, but I think we should also copy the Remarks sections when present:

https://github.com/dotnet/dotnet-api-docs/blob/f6609e53f05d5c00724aaaf5a31120aa5014b79b/xml/System.Net.Sockets/UdpClient.xml#L2324-L2338

(This applies to all new overloads.)

@carlossanlop any plans to make this process simpler and the code less bloated with long copy-pasted blocks? (Not sure if you are the right person to tag about this.)

The long-term plan is to merge the existing external docs back in to the code, so that the code is the one source of truth.

I don't know when/how this will happen; @carlossanlop is the right person to talk to.

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

antonfirsov · 2021-06-21T20:14:46Z

The only remaining issue in the PR is highlighted in #53429 (comment), apart from that, this is ready to merge in my opinion.

@geoffkizer any thoughts on the <remarks> stuff? Strictly speaking we probably have to add the remarks sections, but to me it feels very bad to pollute the code with that, and expect community contributors do do a bunch of chore stuff.

geoffkizer · 2021-06-22T17:35:25Z

I wouldn't worry about <remarks> for now. The ones for existing overloads should be preserved.

But @carlossanlop would know for sure.

antonfirsov · 2021-06-22T19:28:41Z

/azp run runtime-libraries-coreclr outerloop

azure-pipelines · 2021-06-22T19:28:55Z

Azure Pipelines successfully started running 1 pipeline(s).

antonfirsov

LGTM, let's not bother with <remarks>. Test failures are unrelated. @lateapexearlyspeed thanks again!

lateapexearlyspeed · 2021-06-24T05:08:24Z

Thanks @antonfirsov , I am happy to work and finish it :)

…bugger2 * origin/main: (107 commits) Disable MacCatalyst arm64 PR test runs on staging pipeline (dotnet#54678) [WASM] Fix async/await in config loading (dotnet#54652) Fix for heap_use_after_free flagged by sanitizer (dotnet#54679) [wasm] Bump emscripten to 2.0.23 (dotnet#53603) Fix compiler references when building inside VS (dotnet#54614) process more TLS frames at one when available (dotnet#50815) Add PeriodicTimer (dotnet#53899) UdpClient with span support (dotnet#53429) exclude fragile tests (dotnet#54671) get last error before calling a method that might fail as well (dotnet#54667) [FileStream] add tests for device and UNC paths (dotnet#54545) Fix sporadic double fd close (dotnet#54660) Remove Version.Clone from AssemblyName.Clone (dotnet#54621) [wasm] Enable fixed libraries tests (dotnet#54641) [wasm] Fix blazor/aot builds (dotnet#54651) [mono][wasm] Fix compilation error on wasm (dotnet#54659) Fix telemetry for Socket connects to Dns endpoints (dotnet#54071) [wasm] Build static components; include hot_reload in runtime (dotnet#54568) [wasm][debugger] Reuse debugger-agent on wasm debugger (dotnet#52300) Put Crossgen2 in sync with dotnet#54235 (dotnet#54438) ...

Initial commit: Send() with span.

f05bfbf

dotnet-issue-labeler bot added area-System.Net new-api-needs-documentation labels May 28, 2021

geoffkizer reviewed May 28, 2021

View reviewed changes

CertifiedRice approved these changes May 29, 2021

View reviewed changes

Add other send methods; refactor code by reusing GetEndpoint() for (h…

e5cd921

…ostname, port) overloads.

geoffkizer reviewed Jun 1, 2021

View reviewed changes

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

lateapexearlyspeed added 2 commits June 3, 2021 19:17

Add ReceiveAsync().

32050da

Add method 'SendAsync(System.ReadOnlyMemory<byte> datagram, string? h…

d9c5e2b

…ostname, int port ...); Add test.

lateapexearlyspeed marked this pull request as ready for review June 7, 2021 09:26

antonfirsov reviewed Jun 8, 2021

View reviewed changes

lateapexearlyspeed added 3 commits June 11, 2021 15:26

Fix comment: Add xml doc for new APIs.

d8e144c

Fix comment: Refine test codes.

05b0cf2

Remove tab.

14465d0

karelz assigned antonfirsov Jun 16, 2021

Use ThrowsAnyAsync() to assert.

0ca0fee

antonfirsov reviewed Jun 17, 2021

View reviewed changes

Fix comment: add more test for new SendAsync overloads.

a368fff

antonfirsov approved these changes Jun 23, 2021

View reviewed changes

antonfirsov merged commit b317d06 into dotnet:main Jun 24, 2021

karelz added this to the 6.0.0 milestone Jul 15, 2021

ghost locked as resolved and limited conversation to collaborators Aug 14, 2021

	await Assert.ThrowsAsync<ObjectDisposedException>(() => udpClient.SendAsync(new ReadOnlyMemory<byte>(), remoteEP).AsTask());
	Assert.Throws<ObjectDisposedException>(() => _ = udpClient.SendAsync(new ReadOnlyMemory<byte>(), remoteEP));

		/// <exception cref="SocketException">An error occurred when accessing the socket.</exception>
		public ValueTask<int> SendAsync(ReadOnlyMemory<byte> datagram, CancellationToken cancellationToken = default) =>

UdpClient with span support #53429

UdpClient with span support #53429

Conversation

lateapexearlyspeed commented May 28, 2021 • edited Loading

dotnet-issue-labeler bot commented May 28, 2021

ghost commented May 28, 2021

antonfirsov commented May 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoffkizer commented Jun 1, 2021

lateapexearlyspeed commented Jun 2, 2021

geoffkizer commented Jun 2, 2021

lateapexearlyspeed commented Jun 7, 2021

geoffkizer commented Jun 8, 2021

antonfirsov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lateapexearlyspeed Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lateapexearlyspeed commented Jun 15, 2021

antonfirsov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonfirsov commented Jun 21, 2021

geoffkizer commented Jun 22, 2021

antonfirsov commented Jun 22, 2021

azure-pipelines bot commented Jun 22, 2021

antonfirsov left a comment

Choose a reason for hiding this comment

lateapexearlyspeed commented Jun 24, 2021

lateapexearlyspeed commented May 28, 2021 •

edited

Loading

lateapexearlyspeed Jun 15, 2021 •

edited

Loading