-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Socket.SendFileAsync based on SendPacketsAsync #52208
Changes from all commits
6e720ef
d2a7c95
be96cc3
c0aa978
a7eef58
f2fe380
40f9538
dfad0f0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Unix-implementation is also based on SendPackets*, so Windows and Unix got unified here. We can fix that once we base it on SAEA based version. * from #42591 (comment)
|
||
{ | ||
SocketError errorCode = SocketError.Success; | ||
using (fileStream) | ||
{ | ||
// Send the preBuffer, if any | ||
// This will throw on error | ||
if (preBuffer != null && preBuffer.Length > 0) | ||
{ | ||
// Using "this." makes the extension method kick in | ||
await this.SendAsync(new ArraySegment<byte>(preBuffer), SocketFlags.None).ConfigureAwait(false); | ||
} | ||
|
||
// Send the file, if any | ||
if (fileStream != null) | ||
{ | ||
var tcs = new TaskCompletionSource<SocketError>(); | ||
errorCode = SocketPal.SendFileAsync(_handle, fileStream, (_, socketError) => tcs.SetResult(socketError)); | ||
if (errorCode == SocketError.IOPending) | ||
{ | ||
errorCode = await tcs.Task.ConfigureAwait(false); | ||
} | ||
} | ||
} | ||
|
||
if (errorCode != SocketError.Success) | ||
{ | ||
UpdateSendSocketErrorForDisposed(ref errorCode); | ||
UpdateStatusAfterSocketErrorAndThrowException(errorCode); | ||
} | ||
|
||
// Send the postBuffer, if any | ||
// This will throw on error | ||
if (postBuffer != null && postBuffer.Length > 0) | ||
{ | ||
// Using "this." makes the extension method kick in | ||
await this.SendAsync(new ArraySegment<byte>(postBuffer), SocketFlags.None).ConfigureAwait(false); | ||
} | ||
} | ||
|
||
private IAsyncResult BeginSendFileInternal(string? fileName, byte[]? preBuffer, byte[]? postBuffer, TransmitFileOptions flags, AsyncCallback? callback, object? state) | ||
{ | ||
CheckTransmitFileOptions(flags); | ||
|
||
// Open the file, if any | ||
// Open it before we send the preBuffer so that any exception happens first | ||
FileStream? fileStream = OpenFile(fileName); | ||
|
||
return TaskToApm.Begin(SendFileInternalAsync(fileStream, preBuffer, postBuffer), callback, state); | ||
} | ||
|
||
private void EndSendFileInternal(IAsyncResult asyncResult) | ||
{ | ||
TaskToApm.End(asyncResult); | ||
} | ||
} | ||
} |
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.0I 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, see here: #42535
That explains the doc discrepancy -- that change was post-5.0, while the docs are based on 5.0.
Let's be consistent with #42535 here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense it's clear now, thanks!