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 - Perform archive retry when concurrent file access #1889

Merged
merged 1 commit into from Jul 4, 2017

Conversation

snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Jan 5, 2017

Extended Fix for #1887. Perform archive-retry when doing concurrent file-access.

Minor refactoring of BaseMutexFileAppender to make it easier to use. Inheriting from the class activates the archive mutex by default.


This change is Reviewable

@snakefoot snakefoot force-pushed the FileTargetArchiveMutex2 branch 4 times, most recently from d6a56a9 to 130a503 Compare January 6, 2017 00:03
@snakefoot
Copy link
Contributor Author

snakefoot commented Jan 6, 2017

Should probably implement a test to show the archive-mutex is likely to work for #1887 and #1513

@codecov-io
Copy link

codecov-io commented Jan 6, 2017

Codecov Report

Merging #1889 into master will decrease coverage by <1%.
The diff coverage is 50%.

@@           Coverage Diff           @@
##           master   #1889    +/-   ##
=======================================
- Coverage      82%     82%   -<1%     
=======================================
  Files         304     304            
  Lines       21106   21126    +20     
  Branches     2519    2528     +9     
=======================================
- Hits        17264   17247    -17     
- Misses       3211    3248    +37     
  Partials      631     631

@snakefoot snakefoot force-pushed the FileTargetArchiveMutex2 branch 7 times, most recently from 53cb4f3 to 0da6b15 Compare January 7, 2017 12:29
@snakefoot
Copy link
Contributor Author

snakefoot commented Jan 7, 2017

Test implemented, but apparently the archive mutex is not working well on the MONO-Platform (Even when using the Retrying-FileAppender). Disabled the archive test for MONO

And it also seems that the concurrent-tests with async-wrapper is throwing random exceptions. And sometimes the files remains locked after process exit (Failing to cleanup temp-directory after completed test). Disabled throwing exceptions when non-archive test.

@snakefoot
Copy link
Contributor Author

snakefoot commented Jan 7, 2017

Seems named Mutex (and named Semaphore-logic) has been removed on the Linux-platform with MONO ver. 4.4 (and has been disabled by default since MONO ver. 2.8)

I guess a possible solution for the missing Mutex-handling in MONO (and .NET Core) on Linux, could be to create a subfolder in the file-logging-directory. And create a lock-file with the mutex-name for mutual-exclusion.

Maybe an alternative way could be FileStream.Lock() and FileStream.Unlock(). Then the lock-file could be kept open, and not suffer a huge penalty for opening (and closing) the lock-file. Guess the same approach could be used to create a cross-platform FileLockMultiProcessFileAppender.

@snakefoot snakefoot force-pushed the FileTargetArchiveMutex2 branch 6 times, most recently from 8b625b4 to 31d6024 Compare January 7, 2017 23:20
@304NotModified 304NotModified removed this from the 4.4.2 milestone Jan 11, 2017
@304NotModified 304NotModified modified the milestone: 5.0 Feb 3, 2017
@snakefoot snakefoot changed the title FileTarget - Use the Archive Mutex from the old file FileTarget - Perform archive retry when concurrent file access Jun 25, 2017
@snakefoot
Copy link
Contributor Author

snakefoot commented Jun 25, 2017

@304NotModified Synchronized with master, and after further discussions with myself decided not to change the naming of the mutex. Instead I have just changed this PR to allow retry to cater for concurrent file access issues (Like the retrying-file-appender attempts).

@snakefoot
Copy link
Contributor Author

@304NotModified This is no longer a breaking change, and should improve stability of SimpleConcurrentTest (#2198)

Will actually improve stability for all multi-process-file-writing with archive-logic enabled.

@snakefoot snakefoot force-pushed the FileTargetArchiveMutex2 branch 2 times, most recently from 2cfc04a to a734e82 Compare July 3, 2017 16:19
@304NotModified 304NotModified self-assigned this Jul 3, 2017
@304NotModified
Copy link
Member

great work!

@304NotModified 304NotModified modified the milestones: 4.5, 5.0 Jul 4, 2017
@304NotModified 304NotModified added enhancement Improvement on existing feature and removed breaking change Breaking API change (different to semantic change) ready for merge labels Jul 4, 2017
@304NotModified 304NotModified merged commit 7e30493 into NLog:master Jul 4, 2017
@304NotModified 304NotModified modified the milestones: 4.5, 4.5 beta ? Oct 10, 2017
@snakefoot snakefoot deleted the FileTargetArchiveMutex2 branch October 10, 2017 20:42
@snakefoot snakefoot mentioned this pull request Nov 23, 2017
@304NotModified 304NotModified added the file-archiving Issues with archiving with the file target label Aug 26, 2019
@snakefoot snakefoot modified the milestones: 4.5 beta ?, 4.5 Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement on existing feature 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