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.

Sign up for GitHub

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

Jump to bottom

FileTarget: Fix compress archive file not working when using concurrentWrites="True" and keepFileOpen="True" #1183

Merged
merged 5 commits into from
Jan 23, 2016
Merged

FileTarget: Fix compress archive file not working when using concurrentWrites="True" and keepFileOpen="True" #1183

merged 5 commits into from
Jan 23, 2016

Conversation

bhaeussermann
Copy link
Contributor

Fixes #1028

@bhaeussermann
Copy link
Contributor Author

I couldn't find a way to unit-test this as the log file needs to be locked by an external process :(.

@codecov-io
Copy link

Current coverage is 73.75%

Merging #1183 into master will increase coverage by +0.29% as of 9690bcc

@@            master   #1183   diff @@
======================================
  Files          265     265       
  Stmts        15000   15159   +159
  Branches      1647    1666    +19
  Methods          0       0       
======================================
+ Hit          11020   11180   +160
+ Partial        419     418     -1
  Missed        3561    3561

Review entire Coverage Diff as of 9690bcc

Uncovered Suggestions

  1. +0.08% via ...argets/FileTarget.cs#1714...1726
  2. +0.08% via ...c/NLog/LogFactory.cs#181...192
  3. +0.07% via ...gingConfiguration.cs#279...289
  4. See 7 more...

Powered by Codecov. Updated on successful CI builds.

304NotModified
304NotModified reviewed Jan 22, 2016
View reviewed changes
src/NLog/Targets/FileTarget.cs
if (ConcurrentWrites && KeepFileOpen)
{
// Moving the file will cause all other FileTarget's to release the file, unlocking it for deleting.
string tempFileName = Path.GetTempFileName();
Copy link
Member

Choose a reason for hiding this comment

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

I think this is a hack?

@bhaeussermann
FileTarget: Fix compress archive file not working when using concurre…
59602cf 
…ntWrites="True" and keepFileOpen="True"

Instead of moving log file before deleting, rather ensure deleted log file will get released by external process and wait for file delete.
@bhaeussermann
Refactor FileTarget: Use ShouldDeleteOldArchives() method
f6bea66
@bhaeussermann
Copy link
Contributor Author

I couldn't find a way to unit-test this as the log file needs to be locked by an external process :(.

It turns out the other FileTarget need not run in a separate process for the file locking to occur :). I supplemented the existing DateArchive_AllLoggersTransferToCurrentLogFile() test with a new enableArchiveCompression parameter and confirmed that it exposes the bug.

@304NotModified
Copy link
Member

Nice that you could reproduce it with an unit test :)

@304NotModified
Copy link
Member

Thank! It has been merged :)

304NotModified added a commit that referenced this pull request Jan 23, 2016
@304NotModified
Merge pull request #1183 from bhaeussermann/branch0
d62a2fe 
FileTarget: Fix compress archive file not working when using concurrentWrites="True" and keepFileOpen="True"
@304NotModified 304NotModified merged commit d62a2fe into NLog:master Jan 23, 2016
@304NotModified 304NotModified added this to the 4.3 milestone Jan 23, 2016
@304NotModified 304NotModified mentioned this pull request Apr 30, 2016
Closed
@bhaeussermann bhaeussermann mentioned this pull request May 15, 2016
Closed
@304NotModified 304NotModified added the file-archiving Issues with archiving with the file target label Aug 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@304NotModified 304NotModified

Labels
bug Bug report / Bug fix file-archiving Issues with archiving with the file target file-target
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@bhaeussermann @codecov-io @304NotModified