-
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
Use SetFileInformationByHandle on FileStream.SetLength #44170
Conversation
src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems
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.
LGTM. Thanks!
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.
Looks good. I still think we could add at a manual test, as I suggested in the previous PR.
@carlossanlop done. |
@@ -75,5 +75,65 @@ public static void Throw_FileStreamDispose_WhenRemoteMountRunsOutOfSpace() | |||
destinationStream.Dispose(); | |||
}); | |||
} | |||
|
|||
|
|||
const long InitialFileSize = 1024; |
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.
I am not sure whether it is worth adding this tests with the new implementation. The failure path that was there before does not exist anymore.
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.
Wouldn't the test prevent that we don't go back to the previous implementation? However, since it needs to be run manually, I don't think it would catch a potential functional regression before it gets merged.
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.
I think it is very unlikely we would ever go back to the previous implementation.
And if we ever change the implementation again, we should think about test coverage for any interesting failure modes that the change would introduce.
since it needs to be run manually
Right, it makes the test very low value.
As suggested in #43863 (comment), this should prevent
Position
from being set at the moment of callingSetLength
on Windows.