Fix a few Socket.SendFile issues #42535
Conversation
|
Tagging subscribers to this area: @dotnet/ncl |
src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Windows.cs
Show resolved
Hide resolved
src/libraries/System.Net.Sockets/tests/FunctionalTests/SendFile.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Sockets/tests/FunctionalTests/SendFile.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
LGTM with notes @geoffkizer already made.
the manual DangerousAddRef seems unfortunate.
- The string argument in the single-argument overload should be nullable. - All overloads on Windows should allow a null file path, but they've been throwing an exception - On Linux, data was silently truncated when sending a file larger than int.MaxValue with BeginSendFile.
stephentoub
force-pushed
the
fixsendfile
branch
from
28f74a1
to
ce02c8f
Compare
| @@ -383,7 +383,7 @@ public partial class Socket : System.IDisposable | |||
| public int Send(System.ReadOnlySpan<byte> buffer, System.Net.Sockets.SocketFlags socketFlags) { throw null; } | |||
| public int Send(System.ReadOnlySpan<byte> buffer, System.Net.Sockets.SocketFlags socketFlags, out System.Net.Sockets.SocketError errorCode) { throw null; } | |||
| public bool SendAsync(System.Net.Sockets.SocketAsyncEventArgs e) { throw null; } | |||
| public void SendFile(string fileName) { } | |||
| public void SendFile(string? fileName) { } | |||
There was a problem hiding this comment.
I thought we decided in #32675 (comment) that this should be non-null. Calling SendFile(null) won't do anything, so the assumption is you should pass in a fileName.
There was a problem hiding this comment.
If we are moving this to be nullable, to be consistent, we should annotate the BeginSendFile overload as nullable as well.
There was a problem hiding this comment.
We did, but issuing a warning doesn't actually solve any problems here, and it leads to an inconsistency with the other overload. It's not harmful to pass null, it's always been allowed on .NET Framework, etc., so we may as well gain the consistency, avoid potential noisy warnings, and allow null.
There was a problem hiding this comment.
If we are moving this to be nullable, to be consistent, we should annotate the BeginSendFile overload as nullable as well.
I thought I did. I'll fix it.
There was a problem hiding this comment.
(The docs are also explicit about null being allowed.)
ghost
locked as resolved and limited conversation to collaborators
Fixes #42530
Fixes #42529
Fixes #41906