fluffynuts commented on code in PR #319:
URL: https://github.com/apache/logging-log4net/pull/319#discussion_r4015920736
##########
src/log4net/Appender/FileAppender.cs:
##########
@@ -1168,18 +1165,15 @@ protected override void WriteFooter()
/// </remarks>
protected override void WriteHeader()
{
- if (_stream is not null)
+ if (_stream?.AcquireLock() ?? false)
{
- if (_stream.AcquireLock())
+ try
{
- try
- {
- base.WriteHeader();
- }
- finally
- {
- _stream.ReleaseLock();
- }
+ base.WriteHeader();
+ }
+ finally
+ {
+ _stream.ReleaseLock();
Review Comment:
there's an expectation that `_stream` might be null above - but here,
there's no guard - suggest `_stream?.ReleaseLock()`
##########
src/log4net/Appender/FileAppender.cs:
##########
Review Comment:
there is conditional access to _stream above, defending against nulls, but
no such defense here - suggest `_stream?.ReleaseLock()`
##########
src/log4net/Appender/FileAppender.cs:
##########
@@ -1242,6 +1226,28 @@ protected virtual void SafeOpenFile(string fileName,
bool append)
}
}
+ /// <summary>
+ /// Runs <paramref name="action"/> under the file lock if it can be taken,
releasing only what it
+ /// took. It runs unlocked too, because closing has to happen: that is what
frees the handle.
+ /// <see cref="Append(LoggingEvent)"/> and <see cref="WriteHeader"/>
deliberately skip their work
+ /// instead when the lock is refused, so they keep their own acquire.
+ /// </summary>
+ private void RunWithBestEffortLock(Action action)
+ {
+ bool locked = _stream?.AcquireLock() ?? false;
+ try
+ {
+ action();
+ }
+ finally
+ {
+ if (locked)
+ {
+ _stream!.ReleaseLock();
Review Comment:
minor: I know that `_stream` should be non-null, if `locked` is true - but
I'd still recommend using `_stream?.ReleaseLock();` - it unburdens the reader
from figuring that out.
##########
src/log4net/Appender/RollingFileAppender.cs:
##########
@@ -556,9 +566,17 @@ protected virtual void AdjustFileBeforeAppend()
}
}
- if (_rollSize && (File is not null) &&
((CountingQuietTextWriter)QuietWriter!).Count >= MaxFileSize)
+ if (_rollSize && (File is not null)
+ && ((CountingQuietTextWriter)QuietWriter!).Count >= MaxFileSize)
Review Comment:
minor: we can avoid the casts (and the later pattern-match at 644) by saving
our own local reference in `SetQWForFiles`:
```csharp
protected override void SetQWForFiles(TextWriter writer)
=> QuietWriter = _countingWriter = new CountingQuietTextWriter(writer,
ErrorHandler);
private CountingQuoetTextWriter _countingWriter
```
and then use `_countingWriter` here, at 576 and 644 - assuming I haven't
missed something somewhere, but it looks like the only place this can be set is
from the call to `SetQWForFiles` in `FileAppender`?
##########
src/log4net/Appender/RollingFileAppender.cs:
##########
@@ -556,9 +566,17 @@ protected virtual void AdjustFileBeforeAppend()
}
}
- if (_rollSize && (File is not null) &&
((CountingQuietTextWriter)QuietWriter!).Count >= MaxFileSize)
+ if (_rollSize && (File is not null)
+ && ((CountingQuietTextWriter)QuietWriter!).Count >= MaxFileSize)
Review Comment:
also: I would rename `SetQWForFiles` to `SetQuietWriterForFiles` - again, as
the reader, I have to parse what `QW` is when I read the code & the name change
doesn't matter to the compiler.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
