Skip to content
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

Task- and Span-based Socket.SendFile #45132

Closed
wants to merge 9 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/libraries/System.Net.Sockets/ref/System.Net.Sockets.cs
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,9 @@ public static void Select(System.Collections.IList? checkRead, System.Collection
public bool SendAsync(System.Net.Sockets.SocketAsyncEventArgs e) { throw null; }
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; }
public bool SendPacketsAsync(System.Net.Sockets.SocketAsyncEventArgs e) { throw null; }
public int SendTo(byte[] buffer, int offset, int size, System.Net.Sockets.SocketFlags socketFlags, System.Net.EndPoint remoteEP) { throw null; }
public int SendTo(byte[] buffer, int size, System.Net.Sockets.SocketFlags socketFlags, System.Net.EndPoint remoteEP) { throw null; }
Expand Down Expand Up @@ -443,6 +446,7 @@ public enum SocketAsyncOperation
Send = 7,
SendPackets = 8,
SendTo = 9,
SendFile = 10,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed or shall Send be used for SendFileAsync?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are not exposing the SocketAsyncEventArgs-based overload publicly there is no point extending the public API with this option, so let's remove it I guess. (Adding a comment about the workaround when SocketAsyncOperation.Send is being assigned.) @geoffkizer agreed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SocketAsyncEventArgs already has SendPackets, which is effectively a more general version of SendFile. So we shouldn't be adding SendFile to this enum.

I suppose it does raise the question of whether we should have a Task-based SendPacketsAsync method, though...

}
[System.FlagsAttribute]
public enum SocketFlags
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,6 @@
<Compile Include="System\Net\Sockets\SocketAsyncEventArgs.Windows.cs" />
<Compile Include="System\Net\Sockets\IOControlKeepAlive.Windows.cs" />
<Compile Include="System\Net\Sockets\SocketPal.Windows.cs" />
<Compile Include="System\Net\Sockets\TransmitFileAsyncResult.Windows.cs" />
<Compile Include="System\Net\Sockets\UnixDomainSocketEndPoint.Windows.cs" />
<Compile Include="$(CommonPath)System\Net\ContextAwareResult.Windows.cs"
Link="Common\System\Net\ContextAwareResult.Windows.cs" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ public partial class Socket
/// <summary>Cached instance for send operations that return <see cref="Task{Int32}"/>.</summary>
private TaskSocketAsyncEventArgs<int>? _multiBufferSendEventArgs;

/// <summary>Cached instance for file send operations that return <see cref="ValueTask"/>.</summary>
private FileSendSocketAsyncEventargs? _fileSendEventArgs;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just use one of the existing send instances (above) here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the current state of the PR no, as FileSendSocketAsyncEventargs is special to windows and potential parent classes are sealed. Unsealing them might change perf-characteristics or would this be OK?

But I'll try something to unifiy this. Adding the fields to SocketAsyncEventArgs and use AwaitableSocketAsyncEventArgs, then one field could be re-used.


internal Task<Socket> AcceptAsync(Socket? acceptSocket)
{
// Get any cached SocketAsyncEventArg we may have.
Expand Down Expand Up @@ -356,6 +359,31 @@ internal Task<int> SendToAsync(ArraySegment<byte> buffer, SocketFlags socketFlag
return tcs.Task;
}

public ValueTask SendFileAsync(string? fileName, CancellationToken cancellationToken = default)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doc comments are in gfoidl@feade37

Will merge them once the rest of the PR is OK to avoid unnecessary CI builds.

{
return SendFileAsync(fileName, default, default, TransmitFileOptions.UseDefaultWorkerThread, cancellationToken);
}

public ValueTask SendFileAsync(string? fileName, ReadOnlyMemory<byte> preBuffer, ReadOnlyMemory<byte> postBuffer, TransmitFileOptions flags, CancellationToken cancellationToken = default)
{
ThrowIfDisposed();

if (!Connected)
{
throw new NotSupportedException(SR.net_notconnected);
}

if (cancellationToken.IsCancellationRequested)
{
return ValueTask.FromCanceled(cancellationToken);
}

if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, $"::SendFileAsync() SRC:{LocalEndPoint} DST:{RemoteEndPoint} fileName:{fileName}");

FileStream? fileStream = OpenFile(fileName);
return SendFileInternalAsync(fileStream, preBuffer, postBuffer, flags, cancellationToken);
}

private static void ValidateBufferArguments(byte[] buffer, int offset, int size)
{
if (buffer == null)
Expand Down Expand Up @@ -563,6 +591,7 @@ private void DisposeCachedTaskSocketAsyncEventArgs()
Interlocked.Exchange(ref _multiBufferSendEventArgs, null)?.Dispose();
Interlocked.Exchange(ref _singleBufferReceiveEventArgs, null)?.Dispose();
Interlocked.Exchange(ref _singleBufferSendEventArgs, null)?.Dispose();
Interlocked.Exchange(ref _fileSendEventArgs, null)?.Dispose();
}

/// <summary>A TaskCompletionSource that carries an extra field of strongly-typed state.</summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
using System.IO;
using System.Threading.Tasks;
using System.Runtime.Versioning;
using System.Threading;
using System.Runtime.CompilerServices;
using System.Threading.Tasks.Sources;

namespace System.Net.Sockets
{
Expand Down Expand Up @@ -195,7 +198,7 @@ private static void CheckTransmitFileOptions(TransmitFileOptions flags)
}
}

private void SendFileInternal(string? fileName, byte[]? preBuffer, byte[]? postBuffer, TransmitFileOptions flags)
private void SendFileInternal(string? fileName, ReadOnlySpan<byte> preBuffer, ReadOnlySpan<byte> postBuffer, TransmitFileOptions flags)
{
CheckTransmitFileOptions(flags);

Expand All @@ -208,7 +211,7 @@ private void SendFileInternal(string? fileName, byte[]? preBuffer, byte[]? postB
{
// Send the preBuffer, if any
// This will throw on error
if (preBuffer != null && preBuffer.Length > 0)
if (!preBuffer.IsEmpty)
{
Send(preBuffer);
}
Expand All @@ -224,55 +227,66 @@ private void SendFileInternal(string? fileName, byte[]? preBuffer, byte[]? postB
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)
if (!postBuffer.IsEmpty)
{
Send(postBuffer);
}
}

private async Task SendFileInternalAsync(FileStream? fileStream, byte[]? preBuffer, byte[]? postBuffer)
private async ValueTask SendFileInternalAsync(FileStream? fileStream, ReadOnlyMemory<byte> preBuffer, ReadOnlyMemory<byte> postBuffer, TransmitFileOptions flags = TransmitFileOptions.UseDefaultWorkerThread, CancellationToken cancellationToken = default)
{
CheckTransmitFileOptions(flags);

SocketError errorCode = SocketError.Success;
using (fileStream)
{
// Send the preBuffer, if any
// This will throw on error
if (preBuffer != null && preBuffer.Length > 0)
if (!preBuffer.IsEmpty)
{
// Using "this." makes the extension method kick in
await this.SendAsync(new ArraySegment<byte>(preBuffer), SocketFlags.None).ConfigureAwait(false);
await SendAsync(preBuffer, SocketFlags.None, cancellationToken).ConfigureAwait(false);
}

// Send the file, if any
if (fileStream != null)
{
var tcs = new TaskCompletionSource<SocketError>();
errorCode = SocketPal.SendFileAsync(_handle, fileStream, (_, socketError) => tcs.SetResult(socketError));
AsyncValueTaskMethodBuilder<SocketError> taskBuilder = AsyncValueTaskMethodBuilder<SocketError>.Create();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of the TaskCompletionSource this one is used and the callback (below) updated to take a state, so no closures need to be allocated.

ValueTask<SocketError> task = taskBuilder.Task;
errorCode = SocketPal.SendFileAsync(_handle, fileStream, taskBuilder, static (state, _, socketError) => state.SetResult(socketError), cancellationToken);
if (errorCode == SocketError.IOPending)
{
errorCode = await tcs.Task.ConfigureAwait(false);
errorCode = await task.ConfigureAwait(false);
}
}
}

if (errorCode != SocketError.Success)
{
UpdateSendSocketErrorForDisposed(ref errorCode);
UpdateStatusAfterSocketErrorAndThrowException(errorCode);

if (errorCode == SocketError.OperationAborted)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of checking for OperationAborted, I think we should just check if the cancellationToken is cancelled and throw if it is (i.e. call cancellationToken.ThrowIfCancellationRequested)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though we may want to call UpdateStatusAfterSocketError too, not sure.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we may want to call UpdateStatusAfterSocketError too, not sure.

I'm not sure either, but I think we won't.

internal void UpdateStatusAfterSocketError(SocketError errorCode)
{
// If we already know the socket is disconnected
// we don't need to do anything else.
if (NetEventSource.Log.IsEnabled()) NetEventSource.Error(this, $"errorCode:{errorCode}");
if (_isConnected && (_handle.IsInvalid || (errorCode != SocketError.WouldBlock &&
errorCode != SocketError.IOPending && errorCode != SocketError.NoBufferSpaceAvailable &&
errorCode != SocketError.TimedOut)))
{
// The socket is no longer a valid socket.
if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, "Invalidating socket.");
SetToDisconnected();
}
}
would disconnect the socket, and cancellation is no reason to disconnect, or?

{
// Don't wrap into a SocketException
throw new OperationCanceledException(cancellationToken);
}
else
{
UpdateStatusAfterSocketErrorAndThrowException(errorCode);
}
}

// Send the postBuffer, if any
// This will throw on error
if (postBuffer != null && postBuffer.Length > 0)
if (!postBuffer.IsEmpty)
{
// Using "this." makes the extension method kick in
await this.SendAsync(new ArraySegment<byte>(postBuffer), SocketFlags.None).ConfigureAwait(false);
await SendAsync(postBuffer, SocketFlags.None, cancellationToken).ConfigureAwait(false);
}
}

Expand All @@ -284,12 +298,15 @@ private IAsyncResult BeginSendFileInternal(string? fileName, byte[]? preBuffer,
// 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);
return TaskToApm.Begin(SendFileInternalAsync(fileStream, preBuffer, postBuffer).AsTask(), callback, state);
}

private void EndSendFileInternal(IAsyncResult asyncResult)
{
TaskToApm.End(asyncResult);
}

internal sealed class FileSendSocketAsyncEventargs : SocketAsyncEventArgs
{ }
}
}
Loading