Copilot commented on code in PR #319:
URL: https://github.com/apache/logging-log4net/pull/319#discussion_r4041734115
##########
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:
This failed-rename state can also be created by `ExistingInit()` when
`AppendToFile` is false. Control then reaches `base.ActivateOptions()`, which
opens the same source with `append: false` and truncates the file whose rename
just failed, so the data-loss fix does not cover startup rollover. The
activation path needs to reopen with append semantics while this pending rename
exists and schedule its retry.
##########
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:
A pending time-based rename is never retried when `RollingStyle` is `Date`,
because `_rollSize` is false and this outer guard prevents reaching
`RetryFailedRoll`. After the next date boundary the pending rename is
discarded, so the accumulated file is archived under the later period instead
of retrying the original destination. Check pending retries independently of
whether normal size rolling is enabled.
##########
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:
Changelog entries for an existing pull request use the attribution form
`fixed by @... in https://github.com/apache/logging-log4net/pull/319[#319]`, as
illustrated by
`src/changelog/3.5.0/162-quiet-app-settings-in-a-native-host.xml:14`. Append
the PR link here so the generated changelog preserves the implementation
reference.
##########
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:
Unix limits mutex names by encoded byte length, but `string.Length` counts
UTF-16 code units. A resolved path containing multibyte characters can
therefore pass this check while exceeding 255 UTF-8 bytes, and `new Mutex` will
still fail during activation. Measure the UTF-8 byte count before deciding
whether to hash.
##########
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:
Changelog entries for an existing pull request use the attribution form
`fixed by @... in https://github.com/apache/logging-log4net/pull/319[#319]`, as
illustrated by
`src/changelog/3.5.0/162-quiet-app-settings-in-a-native-host.xml:14`. Append
the PR link here so the generated changelog preserves the implementation
reference.
##########
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:
These public overrides in the new hand-written locking fake lack the XML
documentation required for public and protected test members. Add `///
<inheritdoc/>` to each override in this fake so they follow the inherited
`LockingModelBase` contract.
##########
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:
Changelog entries for an existing pull request use the attribution form
`fixed by @... in https://github.com/apache/logging-log4net/pull/319[#319]`, as
illustrated by
`src/changelog/3.5.0/162-quiet-app-settings-in-a-native-host.xml:14`. Append
the PR link here so the generated changelog preserves the implementation
reference.
##########
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:
Changelog entries for an existing pull request use the attribution form
`fixed by @... in https://github.com/apache/logging-log4net/pull/319[#319]`, as
illustrated by
`src/changelog/3.5.0/162-quiet-app-settings-in-a-native-host.xml:14`. Append
the PR link here so the generated changelog preserves the implementation
reference.
##########
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:
These public overrides in the new hand-written locking fake lack the XML
documentation required for public and protected test members. Add `///
<inheritdoc/>` to each override in this fake so they follow the inherited
`LockingModelBase` contract.
##########
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:
This newly wrapped message uses `+` concatenation, contrary to the
established long-literal convention demonstrated by
`src/log4net/Appender/SmtpAppender.cs:60-64`. Use an interpolated multiline raw
string so future edits do not have to manage fragments.
--
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]
