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

FileTarget - Archive should not fail when ArchiveFileName matches FileName #1886

Merged
merged 1 commit into from
Jan 11, 2017

Conversation

snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Jan 4, 2017

Attempt to fix the following build error, and also resolve this issue #1707

https://ci.appveyor.com/project/nlog/nlog/build/4.4.4171

Was able to reproduce the bug locally, and it seems the following happens:

The test uses ArchiveEvery = FileArchivePeriod.Minute and FileName = ${date:format=yyyyMMddHHmmssfff}.txt. It sleeps 50 ms between each LogEvent to ensure that each LogEvent get their own file.

If the test is executed at exact 12:59:59.999 then it will trigger an extra unexpected archive operation. As one of LogEvent timestamps will then be one-minute older than the creation-time of the previous log-file.

The archive operation fails because the log-filename (based on current-time) and the generated archive-filename (based on file-write-time) becomes the same. Because the archive-filename already exists then it attempts to append the contents of the log-filename to the archive-filename and it of course fails.

We could change the ArchiveEvery to Year for the test DeleteArchiveFilesByDateWithDateName, and then it would only fail at new years eve.

We could also consider to change FileTarget.ArchiveFile() to check if fileName and archiveFileName is the same, and if so then just do nothing but doing a InternalLogger.Info()


This change is Reviewable

@304NotModified
Copy link
Member

Great work! \o/

Copy link
Member

@304NotModified 304NotModified left a comment

Choose a reason for hiding this comment

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

Is this change of test still needed?

@snakefoot
Copy link
Contributor Author

snakefoot commented Jan 4, 2017

Well the test is not depending on FileArchivePeriod.Minutes, so no need to introduce more random behavior than needed.

Added a new test that exercises the new code.

@codecov-io
Copy link

codecov-io commented Jan 4, 2017

Current coverage is 81% (diff: 100%)

Merging #1886 into master will decrease coverage by <1%

@@             master      #1886   diff @@
==========================================
  Files           276        276          
  Lines         18636      18640     +4   
  Methods        2861       2861          
  Messages          0          0          
  Branches       2139       2140     +1   
==========================================
- Hits          15160      15157     -3   
- Misses         2971       2978     +7   
  Partials        505        505          

Sunburst

Powered by Codecov. Last update f3f9643...5fdc97e

@304NotModified
Copy link
Member

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@304NotModified 304NotModified merged commit b8f12a7 into NLog:master Jan 11, 2017
@304NotModified 304NotModified added this to the 4.4.2 milestone Jan 11, 2017
@304NotModified
Copy link
Member

merged :)

@304NotModified 304NotModified added the bug Bug report / Bug fix label Jan 11, 2017
@snakefoot snakefoot deleted the FileTargetArchiveSameFile branch October 10, 2017 20:39
@304NotModified 304NotModified added the file-archiving Issues with archiving with the file target label Aug 26, 2019
@owerkop
Copy link

owerkop commented Dec 11, 2019

So is the warning still valid:
"Warning: when deleting archives files is enabled (e.g. maxArchiveFiles ), the folder of the archives should different than the log files. Otherwise, all content, not just logs, in the folder that is not log archives from the target will be deleted."
(https://github.com/nlog/NLog/wiki/File-target )
?

@snakefoot
Copy link
Contributor Author

snakefoot commented Dec 11, 2019 via email

@snakefoot
Copy link
Contributor Author

snakefoot commented Dec 11, 2019

@owerkop Have updated the Wiki for FileTarget about warning only being relevant for NLog 4.4.13 (and older)

@304NotModified
Copy link
Member

Looks great @snakefoot , thanks! (And valid question @owerkop. Also thanks for that!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug report / Bug fix file-archiving Issues with archiving with the file target file-target
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants