Socket.SendFileAsync based on SendPacketsAsync #52208
Conversation
|
Note regarding the 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. |
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsContributes to #42591 Also see #45132 (comment) (from a previous attempt). /cc: @geoffkizer, @antonfirsov
|
src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs
Outdated
Show resolved
Hide resolved
| @@ -226,61 +226,5 @@ private void SendFileInternal(string? fileName, ReadOnlySpan<byte> preBuffer, Re | |||
| Send(postBuffer); | |||
| } | |||
| } | |||
|
|
|||
| private async Task SendFileInternalAsync(FileStream? fileStream, byte[]? preBuffer, byte[]? postBuffer) | |||
There was a problem hiding this comment.
The Unix-implementation is also based on SendPackets*, so Windows and Unix got unified here.
But I assume that now on Unix there are more allocations happening than before.
We can fix that once we base it on SAEA based version.
* from #42591 (comment)
If we base it on SendPacketsAsync, this would work for both Windows and Unix, which is preferred -- we don't want to have more platform-specific code here if we can avoid it.
src/libraries/System.Net.Sockets/tests/FunctionalTests/SendFile.cs
Outdated
Show resolved
Hide resolved
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
The outerloop CI-failures are not related? |
| @@ -412,6 +412,8 @@ public partial class Socket : System.IDisposable | |||
| public void SendFile(string? fileName) { } | |||
| public void SendFile(string? fileName, byte[]? preBuffer, byte[]? postBuffer, System.Net.Sockets.TransmitFileOptions flags) { } | |||
| public void SendFile(string? fileName, System.ReadOnlySpan<byte> preBuffer, System.ReadOnlySpan<byte> postBuffer, System.Net.Sockets.TransmitFileOptions flags) { } | |||
| public System.Threading.Tasks.ValueTask SendFileAsync(string? fileName, System.ReadOnlyMemory<byte> preBuffer, System.ReadOnlyMemory<byte> postBuffer, System.Net.Sockets.TransmitFileOptions flags, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; } | |||
| public System.Threading.Tasks.ValueTask SendFileAsync(string? fileName, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; } | |||
There was a problem hiding this comment.
@geoffkizer do you know why did nullability change here during the API review (#42591 (comment) -> #42591 (comment)). Does anyone need socket.SendFile(null)?
There was a problem hiding this comment.
socket.SendFile(null)
In this case only the pre- and post-buffer will be sent (if given). Whether anyone needs this or not I can't judge...but don't believe so.
There was a problem hiding this comment.
I'm not quite sure what is going on here...
Existing SendFile and BeginSendFile both have an overload that just takes string? fileName. So presumably we wanted SendFileAsync to match, which makes sense, even though SendFile(null) doesn't seem very useful.
However, the documentation has both this listed as string fileName, i.e. not nullable. E.g. here: https://docs.microsoft.com/en-us/dotnet/api/system.net.sockets.socket.beginsendfile?view=net-5.0
I would suggest we should be consistent with SendFile and BeginSendFile and allow null here. However we should also follow up on the doc issue, and we may want to end up making all of these non-null.
There was a problem hiding this comment.
So presumably we wanted SendFileAsync to match, which makes sense, even though SendFile(null) doesn't seem very useful.
That makes sense it's clear now, thanks!
src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs
Outdated
Show resolved
Hide resolved
ghost
locked as resolved and limited conversation to collaborators
Contributes to #42591
It doesn't fix that issue as per #42591 (comment) cancellation is left out in order to unblock #43845
Also see #45132 (comment) (from a previous attempt).
/cc: @geoffkizer, @antonfirsov