FreeAndNil commented on code in PR #319:
URL: https://github.com/apache/logging-log4net/pull/319#discussion_r4041161968
##########
src/log4net/Appender/FileAppender.cs:
##########
Review Comment:
Done. Also `:1113`, which has the same null-conditional acquire and would
have been left as the odd one out. `:1307` keeps the plain call, since
`_stream` is assigned two lines above it.
##########
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:
Done, same as above.
##########
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:
Done. Agreed the reader should not have to derive it from `locked`.
##########
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:
Taken, but as a derived property rather than a cached field:
```csharp
private CountingQuietTextWriter CountingWriter =>
QuietWriter.EnsureIs<CountingQuietTextWriter>();
```
`QuietWriter` has two other writers: `TextWriterAppender.Writer` assigns a
plain `QuietTextWriter`, and `Reset()` nulls it. A cached field has to be
invalidated in both, which makes it a second source of truth, and this PR
already spent three review rounds on two fields drifting apart. A derived one
cannot go stale. `EnsureIs<>` also names the offender rather than throwing a
bare `InvalidCastException`.
The casts at `:570`, `:576` and `:1325` are gone. `:644` and `:1367` keep
their `is` check deliberately: both run after a reopen, and a refused lock
leaves no writer there, so that check is load-bearing rather than defensive.
##########
src/log4net.Tests/Appender/FileAppenderMutexNameTest.cs:
##########
@@ -0,0 +1,222 @@
+#region Apache License
+//
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to you under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+//
+#endregion
+
+using System;
+using System.IO;
+using System.Reflection;
+using System.Text;
+using System.Threading;
+
+using log4net.Appender;
+using log4net.Core;
+using log4net.Layout;
+using log4net.Util;
+
+using NUnit.Framework;
+
+namespace log4net.Tests.Appender;
+
+/// <summary>The mutex name the file lock and the rolling lock derive from the
log file path.</summary>
+[TestFixture]
+public sealed class FileAppenderMutexNameTest
+{
+ /// <summary>Records what the appender had resolved by the time the locking
model was activated.</summary>
+ private sealed class RecordingLock : FileAppender.LockingModelBase
+ {
+ internal string? FileAtActivation { get; private set; }
+
+ public override void ActivateOptions() => FileAtActivation =
CurrentAppender?.File;
+
+ public override Stream? AcquireLock() => Stream.Null;
+
+ public override void ReleaseLock()
+ { }
+
+ public override void OpenFile(string filename, bool append, Encoding
encoding)
+ { }
+
+ public override void CloseFile()
+ { }
+
+ public override void OnClose()
+ { }
+ }
+
+ private string _directory = string.Empty;
+
+ [SetUp]
+ public void SetUp()
+ {
+ _directory = Path.Combine(Path.GetTempPath(),
Guid.NewGuid().ToString("N"));
Review Comment:
Switched, in both new fixtures. `PeanutButter.Utils` was already in the tree
through `log4net-672`, and it ships `net462` and `netstandard2.0`, so both test
legs are covered. Neither fixture keeps state between tests now; the
roll-failure one also drops its shared error handler for a per-test instance.
One knock-on: `PeanutButter.Utils` as a namespace collides with our test
helper type `Utils` under CA1724, so that is `internal TestUtils` now.
A second commit takes the same treatment to three older fixtures:
`SmtpPickupDirAppenderTest`, which kept its pickup directory in a fixture field
inside the build output, `FileAppenderTest`, which leaked two temp files per
run, and `PatternStringTest`, which tracked its config file by hand. Thanks for
the pointer, it found real litter.
--
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]
