FreeAndNil commented on code in PR #319:
URL: https://github.com/apache/logging-log4net/pull/319#discussion_r4054378994
##########
src/Directory.Build.props:
##########
@@ -34,6 +34,7 @@
<NUnitAnalyzersPackageVersion>4.3.0</NUnitAnalyzersPackageVersion>
<NUnitPackageVersion>4.2.2</NUnitPackageVersion>
<NUnit3TestAdapterPackageVersion>4.6.0</NUnit3TestAdapterPackageVersion>
+ <PeanutButterUtilsPackageVersion>2.0.63</PeanutButterUtilsPackageVersion>
Review Comment:
Bumped to 3.0.437. Still targets net462 and netstandard2.0, and
`AutoTempFolder` is unchanged.
##########
src/log4net/Appender/FileAppender.cs:
##########
@@ -1358,6 +1368,40 @@ protected virtual void SetQWForFiles(TextWriter writer)
/// </remarks>
protected static string ConvertToFullPath(string path) =>
SystemInfo.ConvertToFullPath(path);
+ /// <summary>
+ /// Names the mutex that serialises <paramref name="path"/> between
processes, with
+ /// <paramref name="suffix"/> telling one mutex over the same file from
another.
+ /// </summary>
+ /// <remarks>
+ /// The flattened path, as earlier versions computed it. Only a name the
platform rejects is
+ /// hashed: Unix stops at <see cref="MaxMutexNameLength"/>, Windows has no
limit. Unprefixed, so
+ /// on Windows it coordinates one session.
+ /// </remarks>
+ internal static string MutexNameForPath(string path, string suffix)
+ => MutexNameForPath(path, suffix, SystemInfo.IsWindows ? null :
MaxMutexNameLength);
+
+ /// <summary>Takes the limit rather than deciding it, so both branches are
testable anywhere.</summary>
+ private static string MutexNameForPath(string path, string suffix, int?
maxLength)
+ {
+ string name = path.EnsureNotNull()
+ .Replace("\\", "_")
+ .Replace(":", "_")
+ .Replace("/", "_") + suffix;
+
+ if (maxLength is null || name.Length <= maxLength)
Review Comment:
Right, and measured: 104 characters at 304 bytes throws, 255 ASCII
characters does not. The cap uses
`Encoding.UTF8.GetByteCount` now, pinned by
`AMultibytePathIsMeasuredInBytes`.
##########
src/log4net/Appender/RollingFileAppender.cs:
##########
@@ -556,9 +574,16 @@ protected virtual void AdjustFileBeforeAppend()
}
}
- if (_rollSize && (File is not null) &&
((CountingQuietTextWriter)QuietWriter!).Count >= MaxFileSize)
+ if (_rollSize && (File is not null) && CountingWriter.Count >=
MaxFileSize)
{
- RollOverSize();
+ if (_pendingRename is null)
+ {
+ RollOverSize();
+ }
+ else if (CountingWriter.Count >= _pendingRename.RetryAtCount)
Review Comment:
Right. The check now runs ahead of the size roll and independently of it,
paced by `MaxFileSize`.
Test: `AFailedTimeRenameIsRetriedWithoutSizeRolling`.
##########
src/log4net/Appender/RollingFileAppender.cs:
##########
@@ -1353,7 +1466,11 @@ protected virtual void RollOverRenameFiles(string
baseFileName)
CurrentSizeRollBackups++;
// Rename fileName to fileName.1
- RollFile(baseFileName, CombinePath(baseFileName, ".1"));
+ if (!TryRollFile(baseFileName, CombinePath(baseFileName, ".1")))
+ {
+ CurrentSizeRollBackups--;
+ RecordFailedBaseRename(baseFileName, CombinePath(baseFileName, ".1"),
wasBackupCountReverted: true);
Review Comment:
Right, and the one path the fix missed. Solved a step lower: `OpenFile`
never truncates a file a
pending rename left behind, whatever `AppendToFile` says, which also covers
later reopens and
mutates no user setting. Test:
`AFailedStartupRollKeepsTheFileItCouldNotMove`.
##########
src/changelog/3.5.0/319-lock-level-underflow.xml:
##########
@@ -0,0 +1,13 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance";
+ xmlns="https://logging.apache.org/xml/ns";
+ xsi:schemaLocation="https://logging.apache.org/xml/ns
https://logging.apache.org/xml/ns/log4j-changelog-0.xsd";
+ type="fixed">
+ <issue id="319" link="https://github.com/apache/logging-log4net/pull/319"/>
+ <description format="asciidoc">Stop the file lock counter going negative.
Writing the footer,
+ closing the writer and opening the file released the lock even when
acquiring it had failed, and a
+ negative count made every later acquisition fail. The footer and close paths
share one helper now,
+ which releases only what it took; opening keeps its own acquire, because
wrapping an unlocked
+ stream throws. Nothing was lost by this, because the appender reopens the
file on the next
+ event (audit da18b6fd-f032, fixed by @FreeAndNil)</description>
Review Comment:
Declined. The `<issue>` element above the description already carries the
link. `CLAUDE.md` said
otherwise, which is where the older entries come from; the rule is corrected
instead.
##########
src/changelog/3.5.0/319-mutex-name-length.xml:
##########
@@ -0,0 +1,14 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance";
+ xmlns="https://logging.apache.org/xml/ns";
+ xsi:schemaLocation="https://logging.apache.org/xml/ns
https://logging.apache.org/xml/ns/log4j-changelog-0.xsd";
+ type="fixed">
+ <issue id="319" link="https://github.com/apache/logging-log4net/pull/319"/>
+ <description format="asciidoc">Keep logging to a deep path on Unix. Both the
rolling lock and the
+ inter-process file lock name their mutex after the log file, and Unix
rejects a name longer than
+ 255 characters, throwing out of `ActivateOptions` and taking the appender
with it before anything
+ was written. Such a name is replaced by a hash of the path now. Windows
enforces no
+ length limit at all, and names are left alone there, so the only ones that
change are the ones
+ that used to throw and exclusion against an older version is nowhere
affected (audit
+ da18b6fd-f010, da18b6fd-f031, fixed by @FreeAndNil)</description>
Review Comment:
Declined. The `<issue>` element above the description already carries the
link. `CLAUDE.md` said
otherwise, which is where the older entries come from; the rule is corrected
instead.
##########
src/changelog/3.5.0/319-mutex-resolved-path.xml:
##########
@@ -0,0 +1,15 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance";
+ xmlns="https://logging.apache.org/xml/ns";
+ xsi:schemaLocation="https://logging.apache.org/xml/ns
https://logging.apache.org/xml/ns/log4j-changelog-0.xsd";
+ type="fixed">
+ <issue id="319" link="https://github.com/apache/logging-log4net/pull/319"/>
+ <description format="asciidoc">Let two processes that spell the log path
differently share one
+ inter-process file lock. `InterProcessLock` named its mutex after the
configured path before that
+ path was resolved, so a relative and an absolute spelling of one file took
two different mutexes
+ and excluded nothing. The name comes from the resolved path now. That
resolves a relative path and
+ nothing else: symbolic links, hard links, 8.3 short names, letter case and
UNC versus mapped-drive
+ spellings still produce different names. The name is also unprefixed, which
on Windows makes it
+ per-session, so a service and an interactive process have never coordinated
through it (audit
+ da18b6fd-f031, fixed by @FreeAndNil)</description>
Review Comment:
Declined. The `<issue>` element above the description already carries the
link. `CLAUDE.md` said
otherwise, which is where the older entries come from; the rule is corrected
instead.
##########
src/changelog/3.5.0/319-rollover-keeps-events.xml:
##########
@@ -0,0 +1,15 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance";
+ xmlns="https://logging.apache.org/xml/ns";
+ xsi:schemaLocation="https://logging.apache.org/xml/ns
https://logging.apache.org/xml/ns/log4j-changelog-0.xsd";
+ type="fixed">
+ <issue id="319" link="https://github.com/apache/logging-log4net/pull/319"/>
+ <description format="asciidoc">Keep the log file when a rollover cannot
rename it. The failed rename
+ was reported and the file then reopened without appending, which destroyed
everything it held; a
+ reader holding the file without `FILE_SHARE_DELETE`, such as a backup or
antivirus agent, is enough
+ to cause it. The file is appended to now. Only the rename that failed is
retried, once per
+ `MaxFileSize` of growth, which is the cadence a working rollover would have
had, because a full
+ retry would shift the numbered backups again and lose the oldest one every
time; the file
+ therefore grows past `MaxFileSize` for as long as the rename keeps failing
(audit da18b6fd-f036,
+ fixed by @FreeAndNil)</description>
Review Comment:
Declined. The `<issue>` element above the description already carries the
link. `CLAUDE.md` said
otherwise, which is where the older entries come from; the rule is corrected
instead.
##########
src/log4net.Tests/Appender/FileAppenderMutexNameTest.cs:
##########
@@ -0,0 +1,208 @@
+#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;
+
+using PeanutButter.Utils;
+
+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()
+ { }
Review Comment:
Done, every override in both fakes.
##########
src/log4net.Tests/Appender/LockingStreamTest.cs:
##########
@@ -0,0 +1,143 @@
+#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 log4net.Appender;
+
+using NUnit.Framework;
+
+namespace log4net.Tests.Appender;
+
+/// <summary>The recursion counter inside the private
<c>FileAppender.LockingStream</c>.</summary>
+[TestFixture]
+public sealed class LockingStreamTest
+{
+ /// <summary>Hands out a stream only when told to, so a failed acquisition
can be staged.</summary>
+ private sealed class SwitchableLock : FileAppender.LockingModelBase
+ {
+ internal bool CanAcquire { get; set; }
+
+ internal int ReleaseCount { get; private set; }
+
+ public override Stream? AcquireLock() => CanAcquire ? Stream.Null : null;
+
+ public override void ReleaseLock() => ReleaseCount++;
+
+ public override void OpenFile(string filename, bool append, Encoding
encoding)
+ { }
+
+ public override void CloseFile()
+ { }
+
+ public override void ActivateOptions()
+ { }
+
+ public override void OnClose()
+ { }
Review Comment:
Done, every override in both fakes.
##########
src/log4net/Appender/RollingFileAppender.cs:
##########
@@ -1298,7 +1346,72 @@ protected void RollOverSize()
}
// This will also close the file. This is OK since multiple close
operations are safe.
- SafeOpenFile(_baseFileName!, false);
+ // A failed rename leaves the file in place; appending keeps what it holds.
+ SafeOpenFile(_baseFileName!, ShouldAppendAfterFailedRoll());
+
+ if (_pendingRename is not null)
+ {
+ ScheduleRollRetry();
+ // The failing rename already reported, and OnlyOnceErrorHandler
silences the handler after
+ // the first report, so this one goes through LogLog to survive.
+ LogLog.Error(_declaringType,
+ $"Rolling {_pendingRename.From} failed, so it is kept and appended to.
Only that rename is "
+ + "retried, once per MaxFileSize of growth, so the backups are left
alone.");
Review Comment:
Done. Worth knowing what it costs: the newlines reach the log and only the
first line carries the
`log4net:ERROR` prefix. Accepted, and `CLAUDE.md` now says so.
--
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]
