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.

Sign up for GitHub

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

Jump to bottom

Dont cache file length when handle has been exposed #50424

Merged
merged 3 commits into from
Mar 31, 2021
Merged

Dont cache file length when handle has been exposed #50424

merged 3 commits into from
Mar 31, 2021

Conversation

adamsitnik
Copy link
Member

No description provided.

@adamsitnik adamsitnik requested a review from Jozkee March 30, 2021 15:35
@ghost
Copy link

ghost commented Mar 30, 2021

Tagging subscribers to this area: @carlossanlop
See info in area-owners.md if you want to be subscribed.

Issue Details
Author: adamsitnik
Assignees: -
Labels:

area-System.IO
Milestone: -

@adamsitnik adamsitnik added this to In progress in System.IO - FileStream via automation Mar 30, 2021
@adamsitnik adamsitnik added this to the 6.0.0 milestone Mar 30, 2021
@adamsitnik adamsitnik requested a review from Jozkee March 30, 2021 18:40
This was referenced Mar 30, 2021
Closed
Closed
Jozkee
Jozkee approved these changes Mar 30, 2021
View reviewed changes
Copy link
Member

@Jozkee Jozkee left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

System.IO - FileStream automation moved this from In progress to Reviewer approved Mar 30, 2021
stephentoub
stephentoub reviewed Mar 30, 2021
View reviewed changes
src/libraries/System.IO.FileSystem/tests/FileStream/FileStreamConformanceTests.cs
public async Task LengthIsNotCachedAfterHandleHasBeenExposed(FileAccess fileAccess)
{
using FileStream stream = (FileStream)await CreateStream(Array.Empty<byte>(), fileAccess);
using FileStream createdFromHandle = new FileStream(stream.SafeFileHandle, fileAccess);
Copy link
Member

Choose a reason for hiding this comment

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

Do you also need a test that marks it as exposed part way through rather than immediately?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so. In this test, we have a FileStream that gets exposed by accessing SafeFileHandle and another one that gets exposed by creating it from the handle. The modification of Length is verified in both directions. IMO it's enough.

If you believe that there is a good reason to do it, please let me know why and I'll be happy to send another PR. In the meantime, I am going to merge it as I really want to start testing other repos against the new implementation.

Copy link
Member

Choose a reason for hiding this comment

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

Because you added code that ignores a _length cache if _exposed is true. For both stream and createFromHandle here, you'll never get to the point where you cache _length in Length, because _exposed is true before Length is ever accessed.

@adamsitnik adamsitnik merged commit 4f81910 into dotnet:main Mar 31, 2021
System.IO - FileStream automation moved this from Reviewer approved to Done Mar 31, 2021
@adamsitnik adamsitnik deleted the dontCacheFileLengthWhenHandleHasBeenExposed branch March 31, 2021 06:42
@adamsitnik adamsitnik mentioned this pull request Apr 7, 2021
Merged
9 tasks
@ghost ghost locked as resolved and limited conversation to collaborators Apr 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stephentoub stephentoub stephentoub left review comments

@Jozkee Jozkee Jozkee approved these changes

Assignees
No one assigned
Labels
area-System.IO
Projects
System.IO - FileStream
  
Done
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@adamsitnik @stephentoub @Jozkee