Skip to content

Conversation

@adamsitnik
Copy link
Member

When locking is enabled and the user requests the file to be truncated, it's opened, then locked and then truncated via ftruncate syscall.

When locking is disabled, we can use O_TRUNC when opening the file and avoid the ftruncate syscall.

@adamsitnik adamsitnik added this to the 6.0.0 milestone Jul 10, 2021
@adamsitnik adamsitnik requested review from jozkee and stephentoub July 10, 2021 13:39
@ghost
Copy link

ghost commented Jul 10, 2021

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

When locking is enabled and the user requests the file to be truncated, it's opened, then locked and then truncated via ftruncate syscall.

When locking is disabled, we can use O_TRUNC when opening the file and avoid the ftruncate syscall.

Author: adamsitnik
Assignees: -
Labels:

area-System.IO

Milestone: 6.0.0

Comment on lines +5 to +6
<!-- file locking can't be disabled on Windows -->
<TargetFrameworks>$(NetCoreAppCurrent)-Unix</TargetFrameworks>
Copy link
Member

Choose a reason for hiding this comment

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

Can we instead have this run everywhere, and have the test changed to be:
Assert.Equal(OperatingSystem.IsWindows(), PlatformDetection.IsFileLockingEnabled);
?

Copy link
Member Author

Choose a reason for hiding this comment

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

This project includes all test files that are run for System.IO.FileSystem.Tests.csproj. If we enable it for Windows, we are basically going to run the same tests with the same config twice on Windows. Why would we want to do that?

Copy link
Member

Choose a reason for hiding this comment

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

Why do we want to do it on Unix? The setting controls a minority of functionality. Shouldn't we be more scoped?

Regardless, my point was that your comment asserts this property, which now controls whether multiple tests run, will always be true on Windows. Seems like a good thing to validate with the test you're adding.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why do we want to do it on Unix?

My main motivation is to make sure that file IO actually works as expected when the locking is disabled. It's unlikely that we introduce a bug in this code path, but it's still possible.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, but running all 6000+ tests?

Comment on lines 208 to 209
case FileMode.Open: // Open maps to the default behavior for open(...). No flags needed.
case FileMode.Truncate: // We truncate the file after getting the lock
case FileMode.Truncate when !DisableFileLocking: // We truncate the file after getting the lock
Copy link
Member

Choose a reason for hiding this comment

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

Rather than duplicating the case for Truncate and Create, I'd rather see this be something like:

case FileMode.Truncate:
    if (DisableFileLocking)
    {
        flags |= Interop.Sys.OpenFlags.O_TRUNC; // if we don't lock the file, we can truncate it when opening
    }
    break;

That way, everything about Truncate is contained in one case, and I don't have to go searching for all the different when combinations to figure out whether everything is covered.

Same for the other modes.
}

@adamsitnik adamsitnik requested a review from stephentoub July 10, 2021 15:04
@adamsitnik adamsitnik merged commit 2eb0071 into dotnet:main Jul 10, 2021
@adamsitnik adamsitnik deleted the useMoreOpenFlags branch July 10, 2021 18:38
@ghost ghost locked as resolved and limited conversation to collaborators Aug 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants