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: Fix file archive race-condition. #1646

Merged
merged 4 commits into from
Sep 18, 2016

Conversation

bhaeussermann
Copy link
Contributor

@bhaeussermann bhaeussermann commented Sep 17, 2016

Fix #1608


This change is Reviewable

@304NotModified
Copy link
Member

Guess we don't have a Mutex on Silverlight

@304NotModified 304NotModified added the bug Bug report / Bug fix label Sep 18, 2016
@codecov-io
Copy link

codecov-io commented Sep 18, 2016

Current coverage is 80% (diff: 80%)

Merging #1646 into master will increase coverage by <1%

@@             master      #1646   diff @@
==========================================
  Files           273        273          
  Lines         16699      16727    +28   
  Methods        2653       2660     +7   
  Messages          0          0          
  Branches       1840       1843     +3   
==========================================
+ Hits          13271      13305    +34   
+ Misses         3009       3002     -7   
- Partials        419        420     +1   

Sunburst

Powered by Codecov. Last update b483611...72e4a4e

// The unusual case of the path being too long; let's hash the canonical name,
// so it can be safely shortened and still remain unique
string hash;
using (MD5 md5 = MD5.Create())
Copy link
Member

Choose a reason for hiding this comment

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

not sure if this is the best approach. We could also substring it and add a guid?

Copy link
Member

Choose a reason for hiding this comment

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

ah well, it was already there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this code came from MutexMultiProcessFileAppender. Adding a Guid won't work, because a different process will compute a different Guid for the same file. We need the same file to have the same mutex-name across processes.

Copy link
Member

Choose a reason for hiding this comment

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

ok thanks! :)

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.

One note about the md5

@bhaeussermann
Copy link
Contributor Author

@304NotModified unfortunately, I cannot produce the error from #1608 anymore, even with the old NLog.dll (it stopped happening; I don't know why). We must probably make a hidden package again so viseu can test again.

@304NotModified 304NotModified merged commit 801a75a into NLog:master Sep 18, 2016
@304NotModified 304NotModified added this to the 4.3.9 milestone Sep 18, 2016
@304NotModified 304NotModified mentioned this pull request Dec 4, 2016
@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
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

3 participants