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.

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

DotNet 6 Preview 7: Inconsistent Get/Set Timestamps when Symlinks are Involved #58012

Closed
fitdev opened this issue Aug 24, 2021 · 9 comments
Closed
Milestone

Comments

@fitdev
Copy link

fitdev commented Aug 24, 2021

Not sure if this is new to Preview 7 when Symlinks were introduced, or if this behavior existed for a long time. Basically there are inconsistent results, when for example:

Existing C:\mysymlink.txt symlink file that points to existing physical file C:\realfile.txt:

FileInfo symlink_to_existing_file = new FileInfo(@"C:\mysymlink.txt");

This will set the timestamp on target, i.e. on C:\realfile.txt (not sure if it sets it on immediate or on final target - I only tested it with 1 level of indirection, but suspect it always sets it on the final target):

var new_CreationTimeUtc = DateTime.UtcNow;
symlink_to_existing_file.CreationTimeUtc = new_CreationTimeUtc;

However when we compare the timestamps, the getter still returns "old" value! Why? Because, presumably it reads the value from the symlink itself, and not the target:

symlink_to_existing_file.Refresh();
var are_timestamps_the_same = symlink_to_existing_file.CreationTimeUtc == new_CreationTimeUtc; //should be true, but is actually false!

The issue seems to affect timestamps only (i.e. Creation, LastWrite, and LastAccess times). Attributes seem to always be set on the actual instance, and not on target, which is the way it should be IMHO.

The example above concerns symlinks, FileInfo, and CreationTimeUtc, however the discrepancy extends to DirectoryInfo as well, plus the static helper methods on File and Directory, so both files and directories seem to be affected.

I think this is a serious issue and needs to be fixed before Net 6 ships. I would prefer that the timestamps behave as attributes in all cases, i.e. they are set on / read from the actual file system entry as defined by the path, NOT its target. That way there is consistency with FileAttributes, and on top of that the ability to get/set timestamps on links vs targets separately.

But at the very least the getters/setters should be consistent.

Also I have not tested this with other platforms, so there may be this issue on Linux too.

Lastly, junctions, although not supported, should behave similarly to how FileAttributes behave, i.e. I should be able to set timestamps on the junction like new DirectoryInfo(@"C:\ThisIsAJunctionDir").CreationTimeUtc = DateTime.UtcNow; so that it affects junction only, not the target.

Hopefully @jozkee , @adamsitnik , and @carlossanlop can look into this.

@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.IO untriaged New issue has not been triaged by the area owner labels Aug 24, 2021
@ghost
Copy link

ghost commented Aug 24, 2021

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

Issue Details

Not sure if this is new to Preview 7 when Symlinks were introduced, or if this behavior existed for a long time. Basically there are inconsistent results, when for example:

Existing C:\mysymlink.txt symlink file that points to existing physical file C:\realfile.txt:

FileInfo symlink_to_existing_file = new FileInfo(@"C:\mysymlink.txt");

This will set the timestamp on target, i.e. on C:\realfile.txt (not sure if it sets it on immediate or on final target - I only tested it with 1 level of indirection, but suspect it always sets it on the final target):

var new_CreationTimeUtc = DateTime.UtcNow;
symlink_to_existing_file.CreationTimeUtc = new_CreationTimeUtc;

However when we compare the timestamps, the getter still returns "old" value! Why? Because, presumably it reads the value from the symlink itself, and not the target:

symlink_to_existing_file.Refresh();
var are_timestamps_the_same = symlink_to_existing_file.CreationTimeUtc == new_CreationTimeUtc; //should be true, but is actually false!

The issue seems to affect timestamps only (i.e. Creation, LastWrite, and LastAccess times). Attributes seem to always be set on the actual instance, and not on target, which is the way it should be IMHO.

The example above concerns symlinks, FileInfo, and CreationTimeUtc, however the discrepancy extends to DirectoryInfo as well, plus the static helper methods on File and Directory, so both files and directories seem to be affected.

I think this is a serious issue and needs to be fixed before Net 6 ships. I would prefer that the timestamps behave as attributes in all cases, i.e. they are set on / read from the actual file system entry as defined by the path, NOT its target. That way there is consistency with FileAttributes, and on top of that the ability to get/set timestamps on links vs targets separately.

But at the very least the getters/setters should be consistent.

Also I have not tested this with other platforms, so there may be this issue on Linux too.

Lastly, junctions, although not supported, should behave similarly to how FileAttributes behave, i.e. I should be able to set timestamps on the junction like new DirectoryInfo(@"C:\ThisIsAJunctionDir").CreationTimeUtc = DateTime.UtcNow; so that it affects junction only, not the target.

Hopefully @jozkee , @adamsitnik , and @carlossanlop can look into this.

Author: fitdev
Assignees: -
Labels:

area-System.IO, untriaged

Milestone: -

@fitdev
Copy link
Author

fitdev commented Aug 24, 2021

Also, as a result of the current behavior, setting timestamps on a symlink whose target does not exist leads to an exception. This should not be the case, as the timestamps should be set on the symlink itself.

I suspect that the reason current behavior exists has to do with

using (SafeFileHandle handle = OpenHandle(fullPath, asDirectory))

And what you want is to open a reparse point itself (if it is a reparse point / link), and so should use:
using SafeFileHandle handle = OpenSafeFileHandle(fullPath, Interop.Kernel32.FileOperations.FILE_FLAG_BACKUP_SEMANTICS | Interop.Kernel32.FileOperations.FILE_FLAG_OPEN_REPARSE_POINT); instead.

@jozkee
Copy link
Member

jozkee commented Aug 24, 2021

It is not trivial deciding whether the expected is that symlinks always return their timestamp as that rarely can be considered valuable information. This issue requires some thinking and maybe some investigation to prior-art to take a stance in how metadata should behave with symlinks. Community has had some proposals in how to deal with this, but I think those have divided opinions: #52908.

This is not a regression, the issue has been there for a while. I will close this one as duplicate of #38824.

@jozkee jozkee closed this as completed Aug 24, 2021
@jozkee jozkee removed the untriaged New issue has not been triaged by the area owner label Aug 24, 2021
@fitdev
Copy link
Author

fitdev commented Aug 24, 2021

The problem is not so much that you cannot set timestamps on symlinks, but rather that the results are not consistent between gets and sets. "Gets" are returned from symlinks, but "sets" are set on their targets. Then shouldn't "gets" also return target's timestamps then?

@jozkee
Copy link
Member

jozkee commented Aug 24, 2021

shouldn't "gets" also return target's timestamps then?

Or viceversa, that's the debate. The work in #38824 should account for that.

@fitdev
Copy link
Author

fitdev commented Aug 24, 2021

I understand. But shouldn't the behavior at least be consistent? I mean as far as I am concerned it would be better to simply throw than to present / persist misleading results, which could have serious consequences, as the program ends up doing not what it says.

I mean it's like this: I look at the path, note the timestamps via Getters, decide I want to change them, use Setters to do that, but the timestamps do not change and instead they change somewhere else (and only if I dig maybe I will discover, oh that's a symlink and therefore the target was changed instead).

Of course, I personally, am now aware of this. But 99% of users are not, and those who rely on the code doing what it says may suffer. So, if you cannot properly support this or decide either way, then just throw exception with a message.

Or, at the very, very least, put warnings in xml docs and on docs.microsoft.com reflecting the fact that timestamps are not consistent on symlinks in a very basic way: getters return symlinks' timestamps; but setters set targets' timestamps.

@jozkee jozkee reopened this Aug 24, 2021
@jozkee jozkee added the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Aug 24, 2021
@fitdev
Copy link
Author

fitdev commented Aug 25, 2021

I should also add that I think the current behavior should be considered as leading to data loss. Yes, it is only a minor data loss since it affects timestamps on physical files / dirs, but a data loss nonetheless. If the API leads you to believe it does one thing, but in actuality does the other unexpected thing which hardly anyone would notice (after all not many people expect the symmetry between Getters and Setters, or in the case of static File and Directory between Get... and Set... APIs to be broken).

And while it may be true that the relative number of users affected by this may be slow, as not many people may care about timestamps, still those who do use these APIs obviously do case about timestamps, or else they would not be using these APIs in the first place.

@adamsitnik adamsitnik added this to the Future milestone Aug 26, 2021
@jozkee
Copy link
Member

jozkee commented Aug 30, 2021

FWIW: This has been broken since .NET framework, it is an important issue regardless, IMO we should fix it in next release. Will close this in favor of #38824 since they are pretty much the same issue and in order to keep the conversation in one place.

@jozkee jozkee closed this as completed Aug 30, 2021
@jozkee jozkee removed the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Aug 30, 2021
@fitdev
Copy link
Author

fitdev commented Aug 30, 2021

@jozkee I understand. But please reference this issue there, in particular the getters vs setters inconsistency.

@ghost ghost locked as resolved and limited conversation to collaborators Sep 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants