Socket.Begin*** methods wrapping Task variants do not throw SocketExceptions synchronously

[antonfirsov]

The patterns we started to use while implementing #43845 are silently introducing a breaking change.

Previously, when a native socket method (eg. WSASend) returned an error within a Begin*** call, we were throwing SocketException synchronously:

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

Lines 2492 to 2496 in f0c9552

	IAsyncResult? result = BeginSend(buffer, offset, size, socketFlags, out errorCode, callback, state);
	if (errorCode != SocketError.Success && errorCode != SocketError.IOPending)
	{
	throw new SocketException((int)errorCode);
	}

We are no longer doing this in the methods we refactored to wrap Tasks. We are just forwarding the failed task to an IAsyncResult:

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

Line 2245 in f3b4330

return TaskToApm.Begin(SendAsync(new ReadOnlyMemory<byte>(buffer, offset, size), socketFlags, default).AsTask(), callback, state);

The Task/ValueTask variants are (by design?) not throwing on sync path. This means that the APM exceptions will be only observed when invoking the corresponding End*** calls, even if the error has been reported synchronously.

We should either switch back to old exception behavior in the APM methods, or announce this as a breaking change.

Note that this is a poorly covered functionality. The refactor work did not lead to any test failure until #47781, and even there, we only encountered it one outerloop test.

/cc @geoffkizer

[ghost]

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

Issue Details

---

The patterns we started to use while implementing #43845 are silently introducing a breaking change.

Previously, when a native socket method (eg. WSASend) returned an error within a Begin*** call, we were throwing SocketException synchronously:

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

Lines 2492 to 2496 in f0c9552

	IAsyncResult? result = BeginSend(buffer, offset, size, socketFlags, out errorCode, callback, state);
	if (errorCode != SocketError.Success && errorCode != SocketError.IOPending)
	{
	throw new SocketException((int)errorCode);
	}

We are no longer doing this in the methods we refactored to wrap Task. We are just wrapping the failed task into an IAsyncResult:

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

Line 2245 in f3b4330

return TaskToApm.Begin(SendAsync(new ReadOnlyMemory<byte>(buffer, offset, size), socketFlags, default).AsTask(), callback, state);

The Task/ValueTask variants are (by design?) not throwing on sync path. This means that the APM exceptions will be only observed when invoking the corresponding End*** calls, even if the error has been reported synchronously.

We should either switch back to old exception behavior in the APM methods, or announce this as a breaking change.

Note that this is a poorly covered functionality. The refactor work did not lead to any test failure until #47781, and even there, we only encountered it one outerloop test.

/cc @geoffkizer

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

[geoffkizer]

We should maintain the existing behavior here if it's not too much trouble, i.e. throw from Begin* for sync failure. Again, I think this is counter to the usual IAsyncResult pattern, which is why the helpers don't do this. But it's better to maintain existing behavior.

FWIW, it would also be interesting to look at .NET Framework behavior here. Early in .NET Core (2.0?) we made a change so that operations that complete synchronously at the OS level would actually complete synchronously at the .NET level, i.e. IAsyncResult/Task. I can't recall how exception behavior was handled before this change, though -- it may be that exceptions were still synchronous and were thrown from Begin even before this change. Either way, it would be good to know for sure. If .NET Framework doesn't throw from Begin in these cases, then that's a good reason to do the same for .NET Core, even if that's different than past behavior of .NET Core.

[antonfirsov]

Looks like we are throwing synchronously in .NET Framework:
https://referencesource.microsoft.com/#System/net/System/Net/Sockets/Socket.cs,3430
https://referencesource.microsoft.com/#System/net/System/Net/Sockets/Socket.cs,4066

I also think, we should revert to the old behavior, so triaging this issue to 6.0. I believe the best is if we fix this before pushing in any further overloads.

[stephentoub]

we should revert to the old behavior

FWIW, I wouldn't invest a ton of time and energy into doing so, nor would I jeopardize the behavior the Task-based methods to do so.

[antonfirsov]

I don't think it's a ton of time and energy to reintroduce this for the APM methods. I didn't mean to change the Task-based overloads. Putting back the untriaged label since there is no consensus.

[karelz]

Triage: Looks like regression we introduced during 6.0. Low cost, we should do it and prevent 6.0 from shipping it.
If cost goes higher, we can revisit.

[antonfirsov]

As per discussion under #51212 (comment), I'm closing this in favor of dotnet/dotnet-api-docs#6658.