Reimplement APM UDP Socket methods on top of task variants and consolidate related tests

[antonfirsov]

Contributes to #43845.

• Reimplements (Begin|End)(SendTo|ReceiveFrom|ReceiveMessageFrom) on top of the new Task-based variants, and deletes unused code
• Moves all related argument validation tests from ArgumentValidationTests.cs to SendTo.cs, ReceiveFrom.cs and ReceiveMessageFrom.cs, and extends coverage. Note that the new test code does not validate ArgumentException.ParamName, since it's very inconsistent across all the overloads.

@geoffkizer PTAL.

[ghost]

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

Issue Details

---

Contributes to #43845.

• Reimplements (Begin|End)(SendTo|ReceiveFrom|ReceiveMessageFrom) on top of the new Task-based variants, and deletes unused code
• Moves all related argument validation tests from ArgumentValidationTests.cs to SendTo.cs, ReceiveFrom.cs and ReceiveMessageFrom.cs, and extends coverage. Note that the new test code does not validate ArgumentException.ParamName, since it's very inconsistent across all the overloads.

@geoffkizer PTAL.

Author:	antonfirsov
Assignees:	-
Labels:	area-System.Net.Sockets
Milestone:	-

[gfoidl]

For creating the Memories be consistent and always use the extension method?

[geoffkizer]

I don't understand why we need to do this. We should at least add a comment explaining this. (Similarly below)

[geoffkizer]

I assume all the deleted ArgumentValidationTests got migrated to tests in the appropriate test file?

Seems like it would be nice to do the same for other tests in ArgumentValidationTests.... not for this PR of course.

[geoffkizer]

Just the one issue re remoteEP handling in Begin below... this is probably just a matter of adding a comment, but I'd like to understand it before we merge.

[antonfirsov]

@geoffkizer added some comments. Here is test code checking the mentioned historical behavior, it would fail without the if (t.IsCompletedSuccessfully) stuff:

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

Lines 317 to 327 in fa54293

	IAsyncResult iar = receiver.BeginReceiveMessageFrom(new byte[1], 0, 1, SocketFlags.None, ref remoteEp, null, null);
	if (iar.CompletedSynchronously)
	{
	_output.WriteLine("Completed synchronously, updated endpoint.");
	Assert.Equal(sender.LocalEndPoint, remoteEp);
	}
	else
	{
	_output.WriteLine("Completed asynchronously, did not update endPoint");
	Assert.Equal(anyEp, remoteEp);
	}

It wouldn't make sense to pass remoteEP by ref if we wouldn't update it in case of synchronous completions. Note that this is the vast majority of cases (literally 100% on Windows).

[antonfirsov]

I assume all the deleted ArgumentValidationTests got migrated to tests in the appropriate test file?

Yeah, this is the case, I went through existing tests carefully to make sure there is no loss in coverage.

Seems like it would be nice to do the same for other tests in ArgumentValidationTests.... not for this PR of course.

I have a One Note document listing Sockets.Tests debt (including the existence of ArgumentValidationTests.cs). Maybe worth to turn it into an issue?

[antonfirsov]

/azp run runtime-libraries-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[geoffkizer]

I have a One Note document listing Sockets.Tests debt (including the existence of ArgumentValidationTests.cs). Maybe worth to turn it into an issue?

Yeah, that seems good.

[geoffkizer]

It wouldn't make sense to pass remoteEP by ref if we wouldn't update it in case of synchronous completions. Note that this is the vast majority of cases (literally 100% on Windows).

Yeah, this behavior is pretty weird -- I think this is counter to the standard IAsyncResult pattern. Thanks for clarifying why we need to do this.

[geoffkizer]

A couple of the DualMode SendTo tests are failing, can you look at these?

[antonfirsov]

@geoffkizer it's the same test failing everywhere (I was lazy to run outerloop locally).

The cause is that Socket_BeginSendToV4IPEndPointToV4Host_Throws expects the method to fail with a synchronously thrown SocketException in case WSASendTo returns an error, but our established patterns of wrapping Tasks does not work like this: when a faulted task is returned, we don't throw immediately, but return a faulted IAsyncResult that will result in an exception in the corresponding End*** call instead.

Since the issue is not specific to BeginSendTo, I opened #47905 to address the topic across all methods. I recommend changing the test and not block on the issue in this PR.

[antonfirsov]

/azp run runtime-libraries-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[antonfirsov]

@geoffkizer I pushed a workaround for the test failure (b3e035e). Is the PR is OK to merge in it's current form?

[geoffkizer]

Yep, LGTM

[antonfirsov]

Outer loop test failures are unrelated: #48049, #41606, #1724, #43877