Runtime: FileInfo.CreationTime should throw PlatformNotSupported on platforms where it isn't supported

[billwert]

On most non-Windows platforms, the stat family of functions returns a struct that is missing st_birthtime. We observe this at build time, and only try to get the value in our PAL if we know it is supported. If we know we don't have st_birthtime, we return default(DateTimeOffset).

Here's some output from Ubuntu 16.04:

CreationTime: Monday, January 1, 0001 12:00:00 AM
CreationTimeUtc: Monday, January 1, 0001 12:00:00 AM
LastAccessTime: Saturday, March 4, 2017 3:27:29 PM
LastAccessTimeUtc: Saturday, March 4, 2017 11:27:29 PM
LastWriteTime: Sunday, February 19, 2017 12:21:30 AM
LastWriteTimeUtc: Sunday, February 19, 2017 8:21:30 AM

This should throw PlatformNotSupportedException, as the platform does not support the concept being expressed. It is in fact dangerous behavior the way it is. Imagine a disk cleanup program designed to delete everything older than a week after it backs up everything created this week.

cc: @ianhays @JeremyKuhne

[stephentoub]

Returning default(DateTime) rather than throwing PNSE was an explicit design choice. Whether it was the right or wrong choice, changing it would be a very noticeable breaking change from .NET Core 1.x. (Not trying to shut down discussion, just highlighting that the value of this change would need to be significant enough to overcome that significant negative.)

[billwert]

Can you link where the discussion was captured? I searched a bit but didn't stumble on it.

I realize it would be a breaking change at this point. I worry that this (and other issues like this, should they exist) will be a common source of bugs as people port things to other platforms, and those bugs can be very dangerous.

[stephentoub]

I'm not sure it's on GitHub anywhere (this was implemented before these components moved to GitHub). IIRC (and I may not), this was Mono's behavior _(edit: if it was Mono's behavior, it no longer is; Mono uses last write time if creation time isn't supported)_, which was as good an oracle as any, and there was concern that more code would break from throwing than from returning default(DateTime).

[billwert]

Fair enough. I could believe that apps developed on Mono were built with non-Windows platforms in mind first, and thus less likely to fall to this sort of bug. Code being ported from Windows seems more at risk.

[billwert]

I should note, I noticed this because I was porting a real app to .NET Core so it could run on linux, and this behavior change caused a bug. This isn't just a hypothetical.

[danmosemsft]

Compatibility with desktop is very important to us, and if this turns out to cause problems, one option is quirking it.

[markwilkie]

Interestingly enough, this scenario would affect me as well in my home app. I'm cleaning up files older than x date.... I have since switched to using the LastWriteTime, but it could have been unfortunate for sure.

[ianhays]

This is somewhat related to (but not the same as) the discussion in https://github.com/dotnet/corefx/issues/1728, https://github.com/dotnet/corefx/issues/2369, and https://github.com/dotnet/corefx/issues/1563.

[billwert]

Another thought: A breaking change for 2.0 is arguably the correct thing to do. If someone is using this in a program on a platform where it is not supported, their program is arguably not doing what they intend and they likely have a bug. An exception in this case will make it unambiguous.

[danmosemsft]

I'd be inclined to agree with @billwert as we explicitly rate Desktop compat over 1.x compat. But I'd be interested to hear the take of area owners, and @marek-safar since you mention it's this way in Mono.
/cc @terrajobst.

[stephentoub]

Not arguing either way, but how is this improving desktop compat? The request doesn't affect Windows, and on Unix it would be throwing an exception where desktop doesn't. In terms of arguably doing or not doing what they want, I'm not convinced of that, either. For example, what about tools that just display the information? They'll now potentially crash when they could have happily shown the default values.

[ianhays]

The question is whether a PSNE is more expected than the 0 time for people porting code from the full framework. And if the answer is a yes, then is it a strong enough yes to justify making a serious breaking change for people porting from 1.0.0 or from Mono?

Personally I don't think it is. While I prefer the PSNE and agree with the OP, I don't think the argument is strong enough to seriously break all existing usages of it in .net core and Mono. I could probably be persuaded if the usage patterns or more people chimed in showing that the 0 time is the "wrong" thing to do, though I'm not certain that will be the case. Like Stephen mentioned, there are scenarios where a 0 time is more useful or easier to deal with than a PNSE and like the OP/Mark mentioned there are scenarios where a 0 time is unexpected and harder to deal with. But if we switch decisions now then there is a guaranteed nontrivial amount of existing Unix code that is firmly broken.

[ellismg]

The question is whether a PSNE is more expected than the 0 time for people porting code from the full framework. And if the answer is a yes, then is it a strong enough yes to justify making a serious breaking change for people porting from 1.0.0 or from Mono?

At one point, binary compatibility with the full framework surface area was a goal for .NET Core 2.0. Is that still the case? If so, I think we should bias towards maintaining the behavior full framework had (or throwing where we can't make a reasonable approximation).

I totally get (and am sympathetic to) Steve's argument that apps that just want to display this data and not take action on it would be busted. I could totally see, PowerShell, for example, being horked if we make the change because maybe they can end up surfacing the value of CreationTime. I knpw we've followed this: "hand back dummy data" pattern elsewhere (IIRC we do it in Diagnostics when you want to get information about processes that we just don't have) and I think it a bunch of places it actually makes sense. In this case, it really feels like the common case is you're getting this data because you want to use it.

I'd rather us look at our app compat lab (and maybe also packages on NuGet.org) or source on GitHub to try to get a sense of how often the data is obtained for display vs action is taking on it. If the majority of uses are actually expecting meaningful data to be returned, I think we should strongly consider throwing here.

As Bill and Mark point out, there are real world apps that would do the wrong thing here. Is the set of Unix applications that we would end up breaking actually large? I would have expected Unix applications to not touch this property since in a majority of cases it doesn't give you any reasonable data.

[stephentoub]

What about another option: have creation time be a synonym for last write time if creation time isn't supported as a concept?

[billwert]

I think that's actually worse. The way it is broken now it was at least obvious _something_ had gone wrong with my app, though it took some time to track down. If it used LastWriteTime instead of CreationTime it could have happened to be correct sometimes, but not necessarily always.

I think it is fine for developers to make a fallback choice in this instance. I think doing it in the platform could lead to even more subtle bugs.

I'm sympathetic to the desire to not break existing Mono/1.x apps. OTOH, I'm also of a mind that we should do as much as we can to inform users their programs are not doing what they think they do as early as possible. The compiler is best; deterministic runtime failure is a close second.

Data about how this is used in the ecosystem would certainly help the conversation.

[marek-safar]

@danmosemsft IIRC what Mono does in this case is that it uses LastWriteTime values when CreationTime value is not available.

[stephentoub]

IIRC what Mono does in this case is that it uses LastWriteTime values when CreationTime value is not available.

@marek-safar, have you heard of anyone having problems with this? Folks taking Windows code to Mono and having significant problems because of it?

[marek-safar]

@stephentoub We have had couple of bug reports about that but as this is usually file-system limitation people had to adjust their code if they really wanted to have two distinct values.

[danmosemsft]

@terrajobst, thoughts?
I don't believe 1.x or Mono compatibility should be the primary consideration, to me the most interesting customer scenario is what would make it most likely that my Desktop library would work reasonably on Unix or if that's not possible then throw. And "reasonably" could be any of the three proposals.

[ianhays]

I don't believe 1.x or Mono compatibility should be the primary consideration, to me the most interesting customer scenario is what would make it most likely that my Desktop library would work reasonably on Unix or if that's not possible then throw.

Agreed.

And "reasonably" could be any of the three proposals.

Exactly. And if we determine the three to be equally reasonable for people porting from desktop then we should choose the option that retains the 1.X compat.

[danmosemsft]

I agree with @ianhays. Unless some kind of semi consensus emerges here I guess "no change" will be the result.

[billwert]

I'd like to see this through but am on vacation. Please don't close before next week.

Get Outlook for iOShttps://aka.ms/o0ukef

---

From: Dan Moseley [e-mail]
Sent: Thursday, March 9, 2017 9:52:15 AM
To: dotnet/corefx
Cc: Bill Wert; Mention
Subject: Re: [dotnet/corefx] FileInfo.CreationTime should throw PlatformNotSupported on platforms where it isn't supported (#16782)

I agree with @ianhayshttps://github.com/ianhays. Unless some kind of semi consensus emerges here I guess "no change" will be the result.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHubhttps://github.com/dotnet/corefx/issues/16782#issuecomment-285461598, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AEUEcFIu3Q9NK18SNuRIsa0u9zN__WNOks5rkFhvgaJpZM4MUmwh.

[karelz]

@billwert fine with me. I expected you to push back on the discussion here :)

[JeremyKuhne]

Sorry, been out for the past week. My thinking is in line with @ianhays position.

I also agree that making creation match last modified would be a bad thing. One can look at creation == time(0) and make a decision about availability of creation time. Creation == modified requires additional logic. In addition, when code doesn't allow for no creation time I'd rather see it fall into always vs. never modified.

[ellismg]

For what it's worth, I spent some time looking at some popular packages on NuGet.org that use CreationTime to see how they were using it.

One example is ZipSharpLib, which wants to flow the creation time of a file into the zip archive entry for the zip file it's building. I've seen other packages which include ZipSharpLib either as a dependency or redistribute the code for some reason. So giving back garbage data means we'll create zip files that have "garbage data" in them (Internally the zip file just stores a Last Modified time, and it looks like the factory that creates entries for zip files can be configured by a user to select what file system they they want to set that value to when adding files to an archive).

NUnit uses this property during a fast path to early out of a directory comparison operation. If the value is incorrect it just means that they will end up doing an expensive directory operation more often than they'd like. The change here if this threw an exception would be either to stop considering CreationTime or guard the call with a try catch that would say they were equal (and hence you may need to do an expensive operation).

The scary one to me is in Application Insights. They have an abstraction over a file called a PlatformFile which exposes this value. The only use of this property is to check that a file is not "too old" to be uploaded, as the server rejects old files. In this case, anytime we go down this code path with the behavior as is these files won't be uploaded and an event will be logged saying the file is too old. It wasn't immediately obvious to me how you land in this part of the code (since I have no familiarity with Application Insights) but it certainly seems like the sort of place that you would have a problem and might not notice, especially since if you're running on windows you would have your files uploaded.

For the above, throwing obviously breaks all of the code. You could argue that the behavior for (1) and (2) today are fine (and would be no worse if we moved do a model where creation time == last modified time on platforms that did not support creation time). (3) is pretty bad, since it represents "silent" data loss today (there is an EventSource event that's fired, but it's very unlikely that's not going to help folks consuming this library who just want to add telemetry to their application). Moving to a model where CreationTime == LastModifiedTime likely helps here, since at least you get that two day counter to do something reasonable. It's not clear if there would be additional impacts with a logically old file being treated as "new" just because someone touch'd it or something.

[danmosemsft]

@billwert as this is back. Bill do you guys have data from your compatibility infra to guide us?

[snakefoot]

Know that BirthTime is the most correct value, but instead of doing fallback to LastWriteTime (mtime), then it could be nice it did fallback to LastChangeTime (ctime) as MONO does.

ctime should be the time of when the filesystem-node was created/updated (Thus normally older than mtime).

NLog uses the FileCreationTime to order files by age, and delete the oldest files. This fails horrible when all have the same timestamp (zero). So have now implemented a fallback to mtime, when FileCreationTime is older than 1980, but it would be even better to have ctime.

[billwert]

We now have several examples where the current implementation does the wrong thing in a way that is silent and a developer might not notice.

It seems like we need to make some change. Here's how I see the pros and cons of the options:

| | Pros | Cons / Risks
| --- | --- | --- |
| Throw PSNE | * Developers are made aware of the potential for an issue deterministically
* Developers make the fall back choice for their application | * Code that would have been fine would require a change
* Library developers moving to 2.0 would need to take care |
| Return LastWriteTime or LastAccessTime | * Apps will not crash
* Apps will sometimes behave as the developer intended | * Apps can have silent bad behavior
* The choice the FX makes be wrong for some apps. |
| Make no change | * Apps will not crash | * Apps can have silent bad behavior
* We need a good documentation strategy to highlight this risk to developers |

I'm sympathetic to the desire to have code continue to work without change. With that in mind, however, I am strongly in favor of throwing. This is the kind of property that business logic is based on, which means that it not doing precisely what the developer expects has greater risk than something like console color. I believe it is important to push this sort of error to the developer and have them make the correct decision for their business domain.

[danmosemsft]

@terrajobst hasn't weighed in yet.

[terrajobst]

I've chatted with a few folks on our side (@weshaggard @ericstj @Petermarcu). We all agree that returning default(T) is really bad. It's equally bad to throw because for the most part, apps would likely work when returning a sensible value. Yes, this API has cross-platform implications but there are better ways we can track this, by, for instance, investing in tools like (https://github.com/terrajobst/platform-compat).

@weshaggard thinks we should just return LastWriteTime. While this can also cause issues (files keep getting younger) it's not very likely that apps will completely break with this behavior, as this case is indistinguishable from the file being deleted and recreated. Yes, an app might still break, but it's much better than throwing because that would guarantee the app will break.

Thus, I'd say: let's return LastWriteTime.

[snakefoot]

Thus, I's say: let's return LastWriteTime.

@terrajobst Would LastChangeTime (ctime) not be better on the Linux platform, when birthtime is not available?

[karelz]

@snakefoot what is the difference between LastWriteTime and LastChangeTime? Just reading the semantic names makes me believe it is the same ...

[snakefoot]

@karelz Not an expert on Linux FileSystems, but LastChangeTime (ctime) usually means the file-inode-change-time, while LastWriteTime (mtime) means filecontent-modification-time. But now reading further then it seems that mtime is best, since ctime will always be updated when mtime is updated, and then also on filename- / permission-change.