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

Added Span overloads for Socket.SendFile #47230

Merged
merged 5 commits into from
Jan 25, 2021
Merged
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
1 change: 1 addition & 0 deletions src/libraries/System.Net.Sockets/ref/System.Net.Sockets.cs
Original file line number Diff line number Diff line change
Expand Up @@ -406,6 +406,7 @@ 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 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
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,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 +208,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 @@ -230,7 +230,7 @@ private void SendFileInternal(string? fileName, byte[]? preBuffer, byte[]? postB

// Send the postBuffer, if any
// This will throw on error
if (postBuffer != null && postBuffer.Length > 0)
if (!postBuffer.IsEmpty)
{
Send(postBuffer);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -392,7 +392,7 @@ private Socket GetOrCreateAcceptSocket(Socket? acceptSocket, bool checkDisconnec
return acceptSocket;
}

private void SendFileInternal(string? fileName, byte[]? preBuffer, byte[]? postBuffer, TransmitFileOptions flags)
private void SendFileInternal(string? fileName, ReadOnlySpan<byte> preBuffer, ReadOnlySpan<byte> postBuffer, TransmitFileOptions flags)
{
// Open the file, if any
FileStream? fileStream = OpenFile(fileName);
Expand Down
50 changes: 48 additions & 2 deletions src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1269,10 +1269,57 @@ public int Send(ReadOnlySpan<byte> buffer, SocketFlags socketFlags, out SocketEr

public void SendFile(string? fileName)
{
SendFile(fileName, null, null, TransmitFileOptions.UseDefaultWorkerThread);
SendFile(fileName, ReadOnlySpan<byte>.Empty, ReadOnlySpan<byte>.Empty, TransmitFileOptions.UseDefaultWorkerThread);
}

/// <summary>
/// Sends the file <paramref name="fileName"/> and buffers of data to a connected <see cref="Socket"/> object
/// using the specified <see cref="TransmitFileOptions"/> value.
/// </summary>
/// <param name="fileName">
/// A <see cref="string"/> that contains the path and name of the file to be sent. This parameter can be <see langword="null"/>.
/// </param>
/// <param name="preBuffer">
/// A <see cref="byte"/> array that contains data to be sent before the file is sent. This parameter can be <see langword="null"/>.
/// </param>
/// <param name="postBuffer">
/// A <see cref="byte"/> array that contains data to be sent after the file is sent. This parameter can be <see langword="null"/>.
/// </param>
/// <param name="flags">
/// One or more of <see cref="TransmitFileOptions"/> values.
/// </param>
/// <exception cref="ObjectDisposedException">The <see cref="Socket"/> object has been closed.</exception>
/// <exception cref="NotSupportedException">The <see cref="Socket"/> object is not connected to a remote host.</exception>
/// <exception cref="InvalidOperationException">The <see cref="Socket"/> object is not in blocking mode and cannot accept this synchronous call.</exception>
/// <exception cref="FileNotFoundException">The file <paramref name="fileName"/> was not found.</exception>
Copy link
Member

Choose a reason for hiding this comment

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

Looks like we don't have test for the FileNotFoundException case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, that's missing.

As the tests will be refactored, should we file an issue for this and where these points get collected? Or won't we forget about this anyway?

Copy link
Member

Choose a reason for hiding this comment

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

If one of us manages to PR the test refactor within a week, I wouldn't bother with an issue, otherwise it would be wise to start tracking it.

My main fear is that more extensive testing will discover a bunch of corner cases and platform inconsistencies (or even actual bugs) we'll have to deal with. This is what made things hard with my other PR.

/// <exception cref="SocketException">An error occurred when attempting to access the socket.</exception>
public void SendFile(string? fileName, byte[]? preBuffer, byte[]? postBuffer, TransmitFileOptions flags)
{
SendFile(fileName, preBuffer.AsSpan(), postBuffer.AsSpan(), flags);
geoffkizer marked this conversation as resolved.
Show resolved Hide resolved
}

/// <summary>
/// Sends the file <paramref name="fileName"/> and buffers of data to a connected <see cref="Socket"/> object
/// using the specified <see cref="TransmitFileOptions"/> value.
/// </summary>
/// <param name="fileName">
/// A <see cref="string"/> that contains the path and name of the file to be sent. This parameter can be <see langword="null"/>.
/// </param>
/// <param name="preBuffer">
/// A <see cref="ReadOnlySpan{T}"/> that contains data to be sent before the file is sent. This buffer can be empty.
/// </param>
/// <param name="postBuffer">
/// A <see cref="ReadOnlySpan{T}"/> that contains data to be sent after the file is sent. This buffer can be empty.
/// </param>
/// <param name="flags">
/// One or more of <see cref="TransmitFileOptions"/> values.
/// </param>
/// <exception cref="ObjectDisposedException">The <see cref="Socket"/> object has been closed.</exception>
/// <exception cref="NotSupportedException">The <see cref="Socket"/> object is not connected to a remote host.</exception>
gfoidl marked this conversation as resolved.
Show resolved Hide resolved
/// <exception cref="InvalidOperationException">The <see cref="Socket"/> object is not in blocking mode and cannot accept this synchronous call.</exception>
stephentoub marked this conversation as resolved.
Show resolved Hide resolved
/// <exception cref="FileNotFoundException">The file <paramref name="fileName"/> was not found.</exception>
/// <exception cref="SocketException">An error occurred when attempting to access the socket.</exception>
public void SendFile(string? fileName, ReadOnlySpan<byte> preBuffer, ReadOnlySpan<byte> postBuffer, TransmitFileOptions flags)
{
ThrowIfDisposed();

Expand All @@ -1286,7 +1333,6 @@ public void SendFile(string? fileName, byte[]? preBuffer, byte[]? postBuffer, Tr
if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, $"::SendFile() SRC:{LocalEndPoint} DST:{RemoteEndPoint} fileName:{fileName}");

SendFileInternal(fileName, preBuffer, postBuffer, flags);

}

// Sends data to a specific end point, starting at the indicated location in the buffer.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -291,13 +291,13 @@ public static unsafe SocketError Send(SafeSocketHandle handle, ReadOnlySpan<byte
return SocketError.Success;
}

public static unsafe SocketError SendFile(SafeSocketHandle handle, SafeFileHandle? fileHandle, byte[]? preBuffer, byte[]? postBuffer, TransmitFileOptions flags)
public static unsafe SocketError SendFile(SafeSocketHandle handle, SafeFileHandle? fileHandle, ReadOnlySpan<byte> preBuffer, ReadOnlySpan<byte> postBuffer, TransmitFileOptions flags)
geoffkizer marked this conversation as resolved.
Show resolved Hide resolved
{
fixed (byte* prePinnedBuffer = preBuffer)
fixed (byte* postPinnedBuffer = postBuffer)
{
bool success = TransmitFileHelper(handle, fileHandle, null, preBuffer, postBuffer, flags);
return (success ? SocketError.Success : GetLastSocketError());
bool success = TransmitFileHelper(handle, fileHandle, null, (IntPtr)prePinnedBuffer, preBuffer.Length, (IntPtr)postPinnedBuffer, postBuffer.Length, flags);
return success ? SocketError.Success : GetLastSocketError();
}
}

Expand Down Expand Up @@ -1024,25 +1024,27 @@ private static unsafe bool TransmitFileHelper(
SafeHandle socket,
SafeHandle? fileHandle,
NativeOverlapped* overlapped,
byte[]? preBuffer,
byte[]? postBuffer,
IntPtr pinnedPreBuffer,
int preBufferLength,
IntPtr pinnedPostBuffer,
int postBufferLength,
TransmitFileOptions flags)
{
bool needTransmitFileBuffers = false;
Interop.Mswsock.TransmitFileBuffers transmitFileBuffers = default(Interop.Mswsock.TransmitFileBuffers);
Interop.Mswsock.TransmitFileBuffers transmitFileBuffers = default;

if (preBuffer != null && preBuffer.Length > 0)
if (preBufferLength > 0)
{
needTransmitFileBuffers = true;
transmitFileBuffers.Head = Marshal.UnsafeAddrOfPinnedArrayElement(preBuffer, 0);
transmitFileBuffers.HeadLength = preBuffer.Length;
transmitFileBuffers.Head = pinnedPreBuffer;
transmitFileBuffers.HeadLength = preBufferLength;
}

if (postBuffer != null && postBuffer.Length > 0)
if (postBufferLength > 0)
{
needTransmitFileBuffers = true;
transmitFileBuffers.Tail = Marshal.UnsafeAddrOfPinnedArrayElement(postBuffer, 0);
transmitFileBuffers.TailLength = postBuffer.Length;
transmitFileBuffers.Tail = pinnedPostBuffer;
transmitFileBuffers.TailLength = postBufferLength;
}

bool releaseRef = false;
Expand Down Expand Up @@ -1077,8 +1079,10 @@ public static unsafe SocketError SendFileAsync(SafeSocketHandle handle, FileStre
handle,
fileStream?.SafeFileHandle,
asyncResult.DangerousOverlappedPointer, // SafeHandle was just created in SetUnmanagedStructures
preBuffer,
postBuffer,
preBuffer is not null ? Marshal.UnsafeAddrOfPinnedArrayElement(preBuffer, 0) : IntPtr.Zero,
preBuffer?.Length ?? 0,
postBuffer is not null ? Marshal.UnsafeAddrOfPinnedArrayElement(postBuffer, 0) : IntPtr.Zero,
postBuffer?.Length ?? 0,
flags);

return asyncResult.ProcessOverlappedResult(success, 0);
Expand Down
68 changes: 68 additions & 0 deletions src/libraries/System.Net.Sockets/tests/FunctionalTests/SendFile.cs
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,8 @@ public void Disposed_ThrowsException()
{
s.Dispose();
Assert.Throws<ObjectDisposedException>(() => s.SendFile(null));
Assert.Throws<ObjectDisposedException>(() => s.SendFile(null, null, null, TransmitFileOptions.UseDefaultWorkerThread));
Assert.Throws<ObjectDisposedException>(() => s.SendFile(null, ReadOnlySpan<byte>.Empty, ReadOnlySpan<byte>.Empty, TransmitFileOptions.UseDefaultWorkerThread));
Assert.Throws<ObjectDisposedException>(() => s.BeginSendFile(null, null, null));
Assert.Throws<ObjectDisposedException>(() => s.BeginSendFile(null, null, null, TransmitFileOptions.UseDefaultWorkerThread, null, null));
Assert.Throws<ObjectDisposedException>(() => s.EndSendFile(null));
Expand All @@ -100,11 +102,44 @@ public void NotConnected_ThrowsException()
using (Socket s = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp))
{
Assert.Throws<NotSupportedException>(() => s.SendFile(null));
Assert.Throws<NotSupportedException>(() => s.SendFile(null, null, null, TransmitFileOptions.UseDefaultWorkerThread));
Assert.Throws<NotSupportedException>(() => s.SendFile(null, ReadOnlySpan<byte>.Empty, ReadOnlySpan<byte>.Empty, TransmitFileOptions.UseDefaultWorkerThread));
Assert.Throws<NotSupportedException>(() => s.BeginSendFile(null, null, null));
Assert.Throws<NotSupportedException>(() => s.BeginSendFile(null, null, null, TransmitFileOptions.UseDefaultWorkerThread, null, null));
}
}

[ConditionalTheory]
Copy link
Member

Choose a reason for hiding this comment

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

[PlatformSpecific] is enough to make this conditional, [ConditionalTheory] is used when you need to evaluate bool members at runtime.

[PlatformSpecific(TestPlatforms.Windows)]
[InlineData(true)]
[InlineData(false)]
public async Task UdpConnection_ThrowsException(bool useAsync)
{
// Create file to send
byte[] preBuffer;
byte[] postBuffer;
Fletcher32 sentChecksum;
string filename = CreateFileToSend(size: 1, sendPreAndPostBuffers: false, out preBuffer, out postBuffer, out sentChecksum);

using var client = new Socket(AddressFamily.InterNetwork, SocketType.Dgram, ProtocolType.Udp);
using var listener = new Socket(AddressFamily.InterNetwork, SocketType.Dgram, ProtocolType.Udp);
listener.BindToAnonymousPort(IPAddress.Loopback);

client.Connect(listener.LocalEndPoint);

if (useAsync)
{
await Assert.ThrowsAsync<SocketException>(() => Task.Factory.FromAsync<string>(client.BeginSendFile, client.EndSendFile, filename, null));
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it make sense to also check SocketException.SocketErrorCode?

}
else
{
Assert.Throws<SocketException>(() => client.SendFile(filename));
}

// Clean up the file we created
File.Delete(filename);
}

[Theory]
[InlineData(false, false, false)]
[InlineData(false, false, true)]
Expand Down Expand Up @@ -156,6 +191,39 @@ public async Task SendFile_NoFile_Succeeds(bool useAsync, bool usePreBuffer, boo
Assert.Equal(0, client.Available);
}

[Theory]
[InlineData(false, false)]
[InlineData(false, true)]
[InlineData(true, false)]
[InlineData(true, true)]
public void SendFileSpan_NoFile_Succeeds(bool usePreBuffer, bool usePostBuffer)
{
using var client = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
using var listener = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
listener.BindToAnonymousPort(IPAddress.Loopback);
listener.Listen(1);

client.Connect(listener.LocalEndPoint);
using Socket server = listener.Accept();

server.SendFile(null);
Assert.Equal(0, client.Available);

byte[] preBuffer = usePreBuffer ? new byte[1] : null;
byte[] postBuffer = usePostBuffer ? new byte[1] : null;
int bytesExpected = (usePreBuffer ? 1 : 0) + (usePostBuffer ? 1 : 0);

server.SendFile(null, preBuffer.AsSpan(), postBuffer.AsSpan(), TransmitFileOptions.UseDefaultWorkerThread);

byte[] receiveBuffer = new byte[1];
for (int i = 0; i < bytesExpected; i++)
{
Assert.Equal(1, client.Receive(receiveBuffer));
}

Assert.Equal(0, client.Available);
}

[ActiveIssue("https://github.com/dotnet/runtime/issues/42534", TestPlatforms.Windows)]
[OuterLoop("Creates and sends a file several gigabytes long")]
[Theory]
Expand Down