Implemented review comments given under Pull Request #16
Conversation
services installed on production box are separate process and are configured to write log into single log file. Due to this following two issues are happening, During write/append operation some processes are failing with error “Unable to acquire lock on file. The process cannot access the file because it is being used by another process”. This is because one process acquires lock (thread safe not process safe) for writing into log file and simultaneously another is trying to acquire lock and fails with above error and it just skips writing into log file. During rolling operation, rolled file gets created with less than 1KB size. Thus log entries are lost. Upon investigation we found that, rolling operation is protected by system wide Mutex lock. At the time of rolling, multiple processes may come at the same time for rolling and first process will roll the original file correctly and give a different name to rolled file and re-create blank original file. Now the second process which would have come for rolling will roll the blank original file and overwrite the rolled file created by first process and thus rolled file is losing the data. We locally have fixed above issues by changing latest log4net source code. We have kept locking model as “MinimalLock” which is current configuration in production. During append operation for acquirelock/releaselock we added system wide mutex so that each process will wait for other process to complete append operation. Thus it will not skip log from being written. During rolling operation we added check whether rolling is already happened by some other process. If yes then skip rolling operation. This will resolve rolling file overwrite by other process issue.
Below was question asked in above PR, Question: Would the patch also work without the mutex but only the fix in the RollingFileAppender class? Ans: Yes we have added new configuration <useMutexWithMinimalLock value="false"/> By Default mutex won't be applied with minimal lock unless above config key is set to true.
| /// <see cref="ReleaseLock"/> and <see cref="CloseFile"/>. | ||
| /// </para> | ||
| /// </remarks> | ||
| public abstract void OpenFile(string filename, bool append, Encoding encoding, bool useMutexWithMinimalLock = false); |
There was a problem hiding this comment.
Please do not change the public API of the locking model. This would make all locking model implementations out there incompatible with the next log4net release. Prefer a new locking model instead.
A basic idea of the strategy pattern for the locking model is that the class using the strategy (FileAppender, in our case) does not require any knowledge about a specific strategy. With the new setting MutexWithMinimalLock however, such a locking model specific knowledge is introduced. And it requires changes to all other implementations as well.
|
Good day It's been quite a while since this PR last saw activity. In an attempt to get pull requests under some semblance of This is not because the contributions aren't valuable - it's simply a matter of being the only person spending I encourage you to re-submit the PR against the current main branch if the issue is still significant. Your |
3 participants
Implemented review comments given under Pull Request
#15
Below was question asked in above PR,
Question: Are you aware of that a mutex lock when opening a file has a significant performance impact when using minimal lock scenario?
ANS: Yes we are aware about this. But as our customer want to have multiple processes writing into same file using RollingFileAppender, we choose to implement mutex in minimal lock model. But we have make usage of this mutex configurable.
Question: Would the patch also work without the mutex but only the fix in the RollingFileAppender class?
Ans: Yes we have added new configuration
By Default mutex won't be applied with minimal lock unless above config key is set to true.