SocketTaskExtensions as instance methods

[stephentoub]

In #22919, we agreed to add two new async socket methods, ReceiveAsync and SendAsync, that work with Memory instead of ArraySegment. The existing ArraySegment-based methods were added as extension methods, and thus we added the new methods as extensions, too.

Feedback was provided that these would be better as instance methods. However, we can't just add the new overloads as instance methods; we would also need to add the existing signatures as instance methods, effectively obsoleting the extensions. That's because of how the C# compiler binding works; once it finds an appropriate instance method, it'll no longer consider extension methods, and so the new methods as instance methods can result in compilation errors for existing uses of the existing extensions: an array segment can be implicitly converted to a memory, so it can use the new APIs, but then there will be a different return type (ValueTask<int> instead of Task<int>), and any usage of that return type other than await can fail to compile. Thus code like Task<int> t = s.ReceiveAsync(arraySegment, socketFlags); will now fail to compile due to trying to convert a ValueTask<int> to Task<int>.

So, two choices:

1. Leave the new methods as extensions.
2. Make the new methods be instance methods, and also add corresponding instance methods for the existing extensions.

We need to make and address this for 2.1; otherwise we'll implicitly do (1), since that's what's in dotnet/corefx#24431.

[davidsh]

Make the new methods be instance methods, and also add corresponding instance methods for the existing extensions.

I vote yes for this.

[terrajobst]

@davidsh: If we were to go with option (1), is there any downside? In other words, is there a concrete benefit to making them instance methods (modulo it being more natural/cleaner)? The benefit of a cosmetic improvement is low, considering we would be duplicating API surface.

If were to go with option (2) we'd declare all of the SocketTaskExtensions on Socket and apply EditorBrowsable(Never) to both the type and the methods to make sure it's fully hidden from IntelliSense.

[davidsh]

Option (1) is less desirable. There is a benefit to making them instance methods beyond more natural/cleaner. If they are instance methods then the implementation of them has direct access to the internals of Socket class. That means there are potentially more ways to optimize the perf, etc. Leaving them as extension methods makes them only able to use the public API surface for the extension method implementation. Option (2) is always better.

[stephentoub]

If they are instance methods then the implementation of them has direct access to the internals of Socket class. That means there are potentially more ways to optimize the perf, etc. Leaving them as extension methods makes them only able to use the public API surface for the extension method implementation.

If the extensions are in the same assembly, they can still use internals, as the extensions today are doing. In core, the extensions are just one-liners that call to instance methods:
https://github.com/dotnet/corefx/blob/master/src/System.Net.Sockets/src/System/Net/Sockets/SocketTaskExtensions.cs

[karelz]

FYI: The API review discussion was recorded - see https://youtu.be/BEBq3__WfDc?t=1136 (20 min duration) and https://youtu.be/BEBq3__WfDc?t=3205 (2 min duration)

[terrajobst]

Video

Given that the methods are already instance methods on the implementation, it implementation wise there is no benefit of doing the move. The driving factor here would be if we wanted to expose for customers as instance methods, so, for example, they can derive from Socket and override them. So we'd go with Option 1.

[karelz]

FYI: Another round of the API review discussion was recorded - see https://youtu.be/OZnaGV2omvI?t=48 (5 min duration)