FileTarget - Validate File CreationTimeUtc when non-Windows #1931
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1931 +/- ##
========================================
Coverage ? 81%
========================================
Files ? 285
Lines ? 19554
Branches ? 2288
========================================
Hits ? 15924
Misses ? 3081
Partials ? 549Continue to review full report at Codecov.
|
snakefoot
force-pushed
the
FileTargetFileCreationTime
branch
2 times, most recently
from
c2b8b50
to
285d643
Compare
| @@ -439,7 +452,16 @@ public Mutex GetArchiveMutex(string fileName) | |||
| var fileInfo = new FileInfo(filePath); | |||
| if (fileInfo.Exists) | |||
| { | |||
| return Time.TimeSource.Current.FromSystemTime(fileInfo.GetCreationTimeUtc()); | |||
| result = fileInfo.GetCreationTimeUtc(); | |||
| if (result.Value.Year < 1980) | |||
There was a problem hiding this comment.
could we get rid of the code duplication?
Maybe something like useFallBack(dateTime r) => r.Year < 1980 && !PlatformDetector.IsDesktopWin32 ?
There was a problem hiding this comment.
or at least move the 1980 to a (internal) const.
snakefoot
force-pushed
the
FileTargetFileCreationTime
branch
5 times, most recently
from
a284bd2
to
0abcb1e
Compare
@304NotModified Have now created a helper-method, that handles the fallback logic. |
| #else | ||
| this.CreationTimeUtc = File.GetCreationTime(this.FileName); | ||
| #endif | ||
| this.CreationTimeUtc = FileCharacteristicsHelper.ValidateFileCreationTime(fileInfo, (f) => f.GetCreationTimeUtc(), (f) => f.GetLastWriteTimeUtc()).Value; |
There was a problem hiding this comment.
not trying to be nitpicking, but isn't this infecting the performance? AFAIK lamba's have there costs, and this code is called a lot?
There was a problem hiding this comment.
This code is called when opening the file. So it is only when using the RetryingMultiProcessAppender it is called a lot. But one can usually only allocate 500 FileStream-objects per second.
Also the lambdas are completely static without capture of this, so the compiler should turn them into zero allocations.
Ran a performance profiler of memory allocations using the RetryingMultiProcessAppender, and it within the BaseFileAppender.UpdateCreationTime() then the most expensive was the FileInfo-allocation, and the second most expensive was assigning the CreationTimeUtc where it used DateTime.ToLocalTime().
snakefoot
force-pushed
the
FileTargetFileCreationTime
branch
from
0abcb1e
to
a753bba
Compare
304NotModified
added
file-target
bug
|
partly fixes #1633, as this is merged to master and not (yet) to CoreCLR branch |
Simple fallback to FileLastWriteTimeUtc, when FileCreationTimeUtc is very old (when non-windows). Simple fix for #1633
Also fixes bug in UnixMultiProcessFileAppender, where the fileExists-check was wrongly done after having created the file-descriptor.
This change is