Add Memory-based APIs to Sockets and NetworkStream #24431
Conversation
| @@ -17,4 +17,16 @@ public partial class Socket : System.IDisposable | |||
| public int Send(ReadOnlySpan<byte> buffer, System.Net.Sockets.SocketFlags socketFlags) { throw null; } | |||
| public int Send(ReadOnlySpan<byte> buffer, System.Net.Sockets.SocketFlags socketFlags, out System.Net.Sockets.SocketError errorCode) { throw null; } | |||
| } | |||
|
|
|||
| public partial static class SocketTaskExtensions | |||
There was a problem hiding this comment.
Why are we adding these as extension methods? Why not just add them as new regular methods (i.e. new APIs)?
There was a problem hiding this comment.
In the API review I believe we'd elected to keep them next to where the existing Task-based methods are. I don't have a strong preference, though; @weshaggard, @terrajobst, @KrzysztofCwalina, opinions?
There was a problem hiding this comment.
I think we added them a long time ago as extension methods because we didn't have a good plan for adding new APIs to .NET Core vs. figuring out how to eventually add them to .NET Framework and then .NETStandard.
But I think at this point, we should be adding them as new APIs (not extension methods) just like we're doing for SslStream (ALPN, SNI). I.e. add them to netcore first, then figure out how/when to add them to .net framework so that eventually they can roll into a new netstandard.
There was a problem hiding this comment.
I don't have a preference. Both approaches seem reasonable.
There was a problem hiding this comment.
I just tried it, and realized it's a breaking change unless we also add the existing extension methods as instance methods. Otherwise, the compiler won't consider the extension methods as valid targets once it finds the instance methods of the same name, and due to signature differences, various uses can then fail to compile.
There was a problem hiding this comment.
I've opened https://github.com/dotnet/corefx/issues/24442 to track revisiting this in an API discussion. I'll go ahead with this PR to keep things unblocked.
Commit migrated from dotnet/corefx@b6ef0f1
Fixes https://github.com/dotnet/corefx/issues/22387
Fixes https://github.com/dotnet/corefx/issues/22608
@geoffkizer, I separated https://github.com/dotnet/corefx/issues/24429 out into a separate issue to avoid colliding with the changes you're making in the guts of Socket. These changes stop just below the surface of SocketAsyncEventArgs; once your changes go in, we can then push the use of
Memory<byte>further down, whereas these changes just extract the array from it (and throw for now if there is no array).cc: @geoffkizer, @Priya91, @wfurt, @davidsh, @KrzysztofCwalina, @ahsonkhan