-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Refactor Socket.SendFile tests and extend coverage #47479
Refactor Socket.SendFile tests and extend coverage #47479
Conversation
Tagging subscribers to this area: @dotnet/ncl Issue Details
This is needed to unblock progress on #42591. @geoffkizer @gfoidl PTAL
|
@@ -33,6 +33,8 @@ public abstract class SocketHelperBase | |||
public abstract Task<int> SendAsync(Socket s, ArraySegment<byte> buffer); | |||
public abstract Task<int> SendAsync(Socket s, IList<ArraySegment<byte>> bufferList); | |||
public abstract Task<int> SendToAsync(Socket s, ArraySegment<byte> buffer, EndPoint endpoint); | |||
public abstract Task SendFileAsync(Socket s, string fileName); | |||
public abstract Task SendFileAsync(Socket s, string fileName, ArraySegment<byte> preBuffer, ArraySegment<byte> postBuffer, TransmitFileOptions flags); |
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.
Should these be ReadOnlyMemories instead of the ArraySegment?
The approved API for SendFileAsync is ROM-based, as opposed to e.g. SendAsync which has an overload for ArraySegment.
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.
These are wrappers (not the actual socket API-s). We are using ArraySegment<byte>
so we can also feed the data to sync old array-based overloads from the shared test code.
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.
It is tricky with SocketTestHelperMemoryNativeTask
: in that case we copy the data to an actual pointer-based ReadOnlyMemory<byte>
from the ArraySegment in the helper implementation.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
This looks really nice now.
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.
LGTM.
src/libraries/System.Net.Sockets/tests/FunctionalTests/SendFile.cs
Outdated
Show resolved
Hide resolved
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.
same nit as other PR :)
Otherwise looks great, Thanks!
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
# Conflicts: # src/libraries/System.Net.Sockets/tests/FunctionalTests/SocketTestHelper.cs
/azp run runtime-libraries-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
SendFile.cs
to utilizeSocketTestHelper<T>
Dispose
VS async).This is needed to unblock progress on #42591.
@geoffkizer @gfoidl PTAL