FileSystem.Unix.File.Move uses "rename" in more cases by sonatique · Pull Request #40611 · dotnet/corefx

sonatique · 2019-08-27T09:22:28Z

with the goal of avoiding link/copy when possible, since not reverting to LinkOrCopy when not necessary improves performance on file systems that do not support hard links, such as FAT.

Before this PR "rename" is used only when source and dest file exist and are on the same device and are the same file on that device.

With this PR "rename" is used when source file exist and source and dest are on the same device and either source and dest are the same file on that device (as before) or dest file does not exist (new).

This latter case permits to use "rename" for simply moving a file to an non existing new name on the same device, instead of "LinkOrCopyFile". Consequently, calling File.Move for such cases on file systems that do not support link (e.g. FAT) will result in much faster operations, because copy won't be used.

See mono/mono#16435

src/System.IO.FileSystem/src/System/IO/FileSystem.Unix.cs

sonatique · 2019-08-29T11:40:37Z

Sorry, this doesn't work as expected

…hen possible, improve performance on file systems that do not support hard links, such as FAT

sonatique · 2019-09-06T09:23:34Z

This version (bb59ae4) has been tested and works as expected

dnfclas · 2019-09-06T09:24:25Z

All CLA requirements met.

danmoseley · 2019-09-09T01:03:53Z

@mandawah could you please look at the test failures? Various Move tests failed. You can see by clicking on Details above next to one of the legs, then the "view more details on Azure Pipelines" then the tests tab:
https://dev.azure.com/dnceng/public/_build/results?buildId=340941&view=ms.vss-test-web.build-test-results-tab
they may all just be cases where the tests for the file system watcher need updating (either in behavior or expectations)

sonatique · 2019-09-09T08:19:01Z

Hello @danmosemsft,
I had a look at those failing tests. As far as I can see, fail conditions are triggered by the fact that some FileWatcher events are no longer fired as expected.

For instance: one test expects a "file deleted event" when a file is moved within the same directory. With current master implementation this is indeed the case, since "link then delete" is used. With my PR implementation, we use rename, so of course no file is deleted.

I have to admit that I don't known how to handle this exactly, I am not familiar with corefx "philosophy" regarding test coverage. I feel that the test I took as an example above is written to validate that a given implementation doesn't change, i.e. "File.Move within same directory shall fire deleted event", rather than testing that the file was actually moved. I am ok with this, but this means that this test defines the behavior ("contract") of File.Move, and not only test the functionality.

What I mean is that: if this test is defining the correct expected behavior, i.e. if the test is the specification of File.Move, then I will have to basically add some code to fire deleted event after rename.

If not, then I can change the test (in this case basically I should probably delete it) because it may not make sens to ask for a delete event when the file is simply renamed/moved in general (regardless of however File.Move is actually implemented).

So, my question is: where can I find the specification (or contracts) of File.Move regarding expected FileSystemWatcher events, etc.? Is the specification implicitly defined by those unit tests? Is it safe (I bet not) to change which events are fired?

But then, last question: even in the current implementation, when some conditions are met, we use rename instead of link/delete. In this case we have a move (rename) without deleted event being fired... Is this really well defined and wanted?

Unit Tests seem to be really tailored to this particular current implementation. Shall I change my code to mimic the current implementation regarding which events are fired? Or adapt the test for my implementation, with the risk of breaking contracts?

Sorry for this long post, but I am new here and I need some guidance . Thank you in advance for you reply.

danmoseley · 2019-09-09T10:11:25Z

I'm not sure either. I think the FSW tests have little to do with the implementation of our Move API and instead are designed to validate behavior in the face of the range of possible file system activity caused by any app by whatever means. They should continue to do that.

sonatique · 2019-09-09T13:32:15Z

Thanks for your reply. I understand that your opinion is that I shall add some code to "fake" a delete even though rename does not actually delete a file. Since rename will still also be used for "moving" a file from a name to another name with a different casing, we'll end-up with:

File.Move("aa.txt", "bb.txt") firing deleted events
File.Move("aa.txt", "AA.txt") not firing deleted events

I am not very confortable with this. I have the feeling that File.Move triggering deleted file events is strange in the first place, especially when really moving a file on the same device: we tell the watcher that there is a deleted file, but not that there is a created file, such that it could conclude that there is one less file than before, which would be wrong. I don't know...

I need to think about the best way to resolve this, thanks.

Any by the way I don't know yet how to produce a deleted event out of nothing, I need to look at it.

danmoseley · 2019-09-09T14:32:55Z

@carlossanlop @JeremyKuhne own this area and can give guidance.

JeremyKuhne · 2019-09-09T17:21:06Z

I don't think we should be updating the watcher to fabricate delete events (any more than we already do with renames). I also don't think that we should attempt to make Move generate the same underlying system events. Just update tests to handle the new state on the Unix builds.

We'd be going down a slippery slope trying anything else. We don't control the file system, we should just tell it like it is. @carlossanlop, do you agree and/or have any other thoughts?

carlossanlop

I agree with @JeremyKuhne. We talked about this PR later today, and it doesn't make sense to have a test that expects an OS event to be fired. @mandawah please update the unit test to not expect a delete event anymore.

sonatique · 2019-09-10T08:15:47Z

OK thanks @JeremyKuhne @carlossanlop , I'll do what you said. Please be aware that this may involve simply removing some tests since only one single functionality is tested per test (which is good).
I'll update my PR accordingly, ASAP.

…eSystem.Unix.File.Move implementation that use rename in more cases, avoiding link/copy when possible

maryamariyan · 2019-10-07T19:27:45Z

@carlossanlop @JeremyKuhne is this PR good for merge? It also has the approval.

* FileSystem.Unix.File.Move use rename in more cases, avoid link/copy when possible, improve performance on file systems that do not support hard links, such as FAT * Adapt Unit Tests for accounting FileSystemWatcher events fired by FileSystem.Unix.File.Move implementation that use rename in more cases, avoiding link/copy when possible

alexischr · 2020-03-31T15:47:16Z

src/System.IO.FileSystem/src/System/IO/FileSystem.Unix.cs

            Interop.Sys.FileStatus sourceStat, destStat;
            if (Interop.Sys.LStat(sourceFullPath, out sourceStat) == 0 && // source file exists
-                Interop.Sys.LStat(destFullPath, out destStat) == 0 && // dest file exists
-                sourceStat.Dev == destStat.Dev && // source and dest are on the same device


…395) * FileSystem.Unix.File.Move uses "rename" in more cases (dotnet#40611) * FileSystem.Unix.File.Move use rename in more cases, avoid link/copy when possible, improve performance on file systems that do not support hard links, such as FAT * Adapt Unit Tests for accounting FileSystemWatcher events fired by FileSystem.Unix.File.Move implementation that use rename in more cases, avoiding link/copy when possible * [FileSystem] ReAdd check for same device Co-authored-by: Sylvain <sf@ellisys.com> Co-authored-by: Mitchell Hwang <mihw@microsoft.com>

* FileSystem.Unix.File.Move use rename in more cases, avoid link/copy when possible, improve performance on file systems that do not support hard links, such as FAT * Adapt Unit Tests for accounting FileSystemWatcher events fired by FileSystem.Unix.File.Move implementation that use rename in more cases, avoiding link/copy when possible

…otnet#40611) (#396) * FileSystem.Unix.File.Move uses "rename" in more cases (dotnet#40611) * FileSystem.Unix.File.Move use rename in more cases, avoid link/copy when possible, improve performance on file systems that do not support hard links, such as FAT * Adapt Unit Tests for accounting FileSystemWatcher events fired by FileSystem.Unix.File.Move implementation that use rename in more cases, avoiding link/copy when possible * [FileSystem] ReAdd check for same device Co-authored-by: Sylvain <sf@ellisys.com> Co-authored-by: Mitchell Hwang <mihw@microsoft.com>

…40611) * FileSystem.Unix.File.Move use rename in more cases, avoid link/copy when possible, improve performance on file systems that do not support hard links, such as FAT * Adapt Unit Tests for accounting FileSystemWatcher events fired by FileSystem.Unix.File.Move implementation that use rename in more cases, avoiding link/copy when possible Commit migrated from dotnet/corefx@89b087b

Dotnet-GitSync-Bot added the area-System.IO label Aug 27, 2019

sonatique changed the title ~~FileSystem.Unix.File.Move use "rename" in more cases~~ FileSystem.Unix.File.Move uses "rename" in more cases Aug 27, 2019

stephentoub reviewed Aug 27, 2019

View reviewed changes

src/System.IO.FileSystem/src/System/IO/FileSystem.Unix.cs Outdated Show resolved Hide resolved

sonatique force-pushed the file.move-uses-rename-in-more-cases branch from bd7b3a0 to 5fa5b7a Compare August 28, 2019 07:27

bartonjs requested a review from JeremyKuhne August 28, 2019 16:51

danmoseley requested a review from carlossanlop August 28, 2019 23:38

sonatique force-pushed the file.move-uses-rename-in-more-cases branch 2 times, most recently from e44a9a6 to 0f9ef8c Compare August 29, 2019 11:02

sonatique closed this Aug 29, 2019

FileSystem.Unix.File.Move use rename in more cases, avoid link/copy w…

bb59ae4

…hen possible, improve performance on file systems that do not support hard links, such as FAT

sonatique reopened this Sep 6, 2019

sonatique force-pushed the file.move-uses-rename-in-more-cases branch from 0f9ef8c to bb59ae4 Compare September 6, 2019 09:24

carlossanlop reviewed Sep 9, 2019

View reviewed changes

sonatique force-pushed the file.move-uses-rename-in-more-cases branch from fbdd9b8 to 05f8418 Compare September 11, 2019 16:14

Adapt Unit Tests for accounting FileSystemWatcher events fired by Fil…

95d205c

…eSystem.Unix.File.Move implementation that use rename in more cases, avoiding link/copy when possible

sonatique force-pushed the file.move-uses-rename-in-more-cases branch from 05f8418 to 95d205c Compare September 11, 2019 16:15

JeremyKuhne approved these changes Sep 11, 2019

View reviewed changes

maryamariyan assigned JeremyKuhne and carlossanlop Oct 7, 2019

carlossanlop removed their assignment Oct 7, 2019

carlossanlop unassigned JeremyKuhne Oct 7, 2019

maryamariyan assigned sonatique Oct 7, 2019

carlossanlop approved these changes Oct 7, 2019

View reviewed changes

carlossanlop merged commit 89b087b into dotnet:master Oct 7, 2019

stephentoub added this to the 5.0 milestone Oct 14, 2019

wfurt mentioned this pull request Nov 7, 2019

fix and enable FileSystemWatcher_File_Rename_NotAffectEachOther test #42430

Merged

Taloth mentioned this pull request Feb 1, 2020

Write unit test to verify File.Move deletes source when overwrite is set to true dotnet/runtime#30822

Closed

mdh1418 mentioned this pull request Mar 31, 2020

FileSystem.Unix.File.Move uses "rename" in more cases (#40611) mono/corefx#395

Merged

alexischr reviewed Mar 31, 2020

View reviewed changes

alexischr mentioned this pull request Mar 31, 2020

Change in FileSystem.Unix.File.Move possibly introduced bug dotnet/runtime#34330

Closed

monojenkins mentioned this pull request Apr 3, 2020

[mono-2019-12] FileSystem.Unix.File.Move uses "rename" in more cases (#40611) mono/corefx#396

Merged

Conversation

sonatique commented Aug 27, 2019

Uh oh!

Uh oh!

sonatique commented Aug 29, 2019

Uh oh!

sonatique commented Sep 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnfclas commented Sep 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danmoseley commented Sep 9, 2019

Uh oh!

sonatique commented Sep 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danmoseley commented Sep 9, 2019

Uh oh!

sonatique commented Sep 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danmoseley commented Sep 9, 2019

Uh oh!

JeremyKuhne commented Sep 9, 2019

Uh oh!

carlossanlop left a comment

Choose a reason for hiding this comment

Uh oh!

sonatique commented Sep 10, 2019

Uh oh!

maryamariyan commented Oct 7, 2019

Uh oh!

alexischr Mar 31, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

sonatique commented Sep 6, 2019 •

edited

Loading

dnfclas commented Sep 6, 2019 •

edited

Loading

sonatique commented Sep 9, 2019 •

edited

Loading

sonatique commented Sep 9, 2019 •

edited

Loading