-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Fix: Socket.SendFile throws when Blocking=false #122831
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
Conversation
|
@dotnet-policy-service agree |
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.
Pull request overview
This PR fixes issue #47287 by adding validation to ensure Socket.SendFile throws InvalidOperationException when called on a non-blocking socket, preventing undefined platform-specific behavior (Windows blocks anyway, Linux may send partial data silently).
Key changes:
- Added explicit
Blockingproperty check in the coreSendFileimplementation - Added comprehensive test coverage for both SendFile overloads with non-blocking sockets
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs | Added validation check to throw InvalidOperationException when Blocking == false in the core SendFile method that all overloads delegate to |
| src/libraries/System.Net.Sockets/tests/FunctionalTests/SendFile.cs | Added test to verify InvalidOperationException is thrown for both SendFile overloads when socket is non-blocking |
8f4c925 to
ff15e6e
Compare
liveans
left a comment
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.
Thanks for the PR, it's LGTM!
|
/azp list |
This comment was marked as spam.
This comment was marked as spam.
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
src/libraries/System.Net.Sockets/tests/FunctionalTests/SendFile.cs
Outdated
Show resolved
Hide resolved
The behavior of SendFile on non-blocking sockets is undefined (Windows blocks anyway, Linux may send partial data), so we now throw InvalidOperationException early.
ff15e6e to
16a69df
Compare
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@liveans I see some CI tasks failed although I don't think they are related to my change. Could you advise how to resolve these? |
Yes, they are unrelated. Since they are unrelated and referenced with necessary issues, this is not a blocker for us. CI looks good, merging this, thanks for the contribution! |
Hi, I noticed this issue was unassigned and marked "good first issue" so I took a crack at it. Note this is a breaking change — happy to discuss the approach or add breaking change documentation if needed.
Problem
Socket.SendFiledoes not validate that the socket is in blocking mode before attempting the operation. This leads to undefined, platform-specific behavior:Blockingproperty settingSolution
Added a validation check at the start of
SendFileto throwInvalidOperationExceptionwhenBlocking == false. This follows the same pattern used by other synchronous socket operations that require blocking mode.Testing
NonBlocking_ThrowsInvalidOperationExceptiontest covering bothSendFileoverloadsBreaking Change
This is a breaking change. Code that previously called
SendFileon a non-blocking socket will now throw an exception:Blockingflag will now failThe previous behavior was undefined and platform-inconsistent, so failing explicitly is preferable to silent misbehavior.
Fixes #47287