NLog.Targets.AtomicFile for atomic file append #5770
Conversation
|
Caution Review failedThe pull request is closed. ## Walkthrough
This pull request updates several build and test scripts to standardize path formatting while introducing a new AtomicFileTarget for atomic file appending in concurrent, multi-process logging. New projects and test suites for AtomicFile functionality have been added along with namespace and minor variable corrections in existing test files. Additionally, refactorings in network logging components include renaming SyslogSeverity to SyslogLevel and updating associated documentation, method signatures, and preprocessor directives.
## Changes
| File(s) | Change Summary |
|---------|----------------|
| `build.ps1`, `run-tests.ps1` | Standardized path formatting (backslashes to forward slashes) and added commands for packaging and testing the new AtomicFile target. |
| `src/NLog.Targets.AtomicFile/AtomicFileTarget.cs`, `src/NLog.Targets.AtomicFile/NLog.Targets.AtomicFile.csproj`, `src/NLog.sln` | Added a new AtomicFileTarget class for atomic file appending, created a dedicated project, and integrated it into the solution. |
| `tests/NLog.Targets.AtomicFile.Tests/*` | Introduced new test files, including `ConcurrentWritesMultiProcessTests.cs`, `ProcessRunner.cs`, and project files to validate AtomicFile functionality. |
| `tests/NLog.Targets.ConcurrentFile.Tests/*` | Updated namespaces, corrected logger initialization, and fixed variable naming for consistency in concurrent file tests. |
| `src/NLog/Targets/ConsoleTargetHelper.cs`, `src/NLog/Config/ConfigurationItemFactory.cs` | Modified preprocessor directives and added a new method (`SafeRegisterNamedType`) to streamline type registration. |
| `src/NLog.Targets.Network/Layouts/*`, `src/NLog.Targets.Network/Targets/SyslogTarget.cs`, `tests/NLog.Targets.Network.Tests/GelfLayoutTests.cs`, `src/NLog.Targets.Network/Layouts/SyslogLevel.cs` | Renamed SyslogSeverity to SyslogLevel, updated mappings, method signatures, XML documentation, and added a new layout attribute for Log4J. |
| `src/NLog/Targets/ColoredConsoleTarget.cs`, `src/NLog/LayoutRenderers/*` | Added XML documentation generation attributes to several properties to enhance documentation categorization and ordering. |
| `src/NLog/Targets/FileAppenders/ExclusiveFileLockingAppender.cs` | Modified type check condition in `RefreshCountedFileSize` method for stricter type matching. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant L as Logger
participant A as AtomicFileTarget
participant P as Platform Check
participant FS as File System
L->>A: CreateFileStream(filePath, bufferSize)
A->>A: Check ConcurrentWrites & KeepFileOpen
alt ConcurrentWrites not allowed
A->>A: Call base.CreateFileStream(filePath, bufferSize)
else ConcurrentWrites allowed
A->>P: Determine platform (Framework/Windows/Unix)
alt .NET Framework or Windows
A->>FS: Create FileStream with atomic options
else Unix
A->>A: Call CreateUnixStream(filePath)
end
end
A-->>L: Return file streamAssessment against linked issues
Poem
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentWritesMultiProcessTests.cs (1)
55-55: Logger Name InconsistencyThe logger is being obtained with the name
"NLog.UnitTests.Targets.ConcurrentWritesMultiProcessTests", which no longer reflects the current namespace. For clarity and consistency, update the logger name to reflect the new namespace. For example:-private readonly Logger _logger = LogManager.GetLogger("NLog.UnitTests.Targets.ConcurrentWritesMultiProcessTests"); +private readonly Logger _logger = LogManager.GetLogger("NLog.Targets.ConcurrentFile.Tests.ConcurrentWritesMultiProcessTests");tests/NLog.Targets.AtomicFile.Tests/Properties/AssemblyInfo.cs (1)
1-35: New AssemblyInfo and Test ParallelizationThis new file correctly establishes licensing information and introduces the assembly attribute to disable xUnit test parallelization:
[assembly: Xunit.CollectionBehavior(DisableTestParallelization = true)]This is essential for tests that require sequential execution (e.g., due to file access or atomic append scenarios). As an optional improvement, consider adding a brief comment above the attribute explaining why parallelization is disabled to assist future maintainers.
tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentFileTargetTests.cs (1)
34-34: Namespace Update for Test Organization
The namespace declaration has been updated toNLog.Targets.ConcurrentFile.Tests, which aligns with the new organizational structure for tests related to the concurrent file target enhancements. Please verify that all references in the solution (such as project file configurations and other test files) are updated accordingly to reflect this change.tests/NLog.Targets.AtomicFile.Tests/NLog.Targets.AtomicFile.Tests.csproj (1)
4-6: Consider framework coverage gap.The test project targets .NET Framework 4.6.2 as its lowest version, while the main project targets .NET Framework 3.5. Consider if additional test coverage for older frameworks would be beneficial.
You might want to add .NET Framework 3.5 to the test targets if testing lower framework functionality is important:
- <TargetFrameworks Condition=" '$(TargetFrameworks)' == '' AND '$(VisualStudioVersion)' < '17.0' ">net462</TargetFrameworks> - <TargetFrameworks Condition=" '$(TargetFrameworks)' == '' ">net462;net8.0</TargetFrameworks> + <TargetFrameworks Condition=" '$(TargetFrameworks)' == '' AND '$(VisualStudioVersion)' < '17.0' ">net462</TargetFrameworks> + <TargetFrameworks Condition=" '$(TargetFrameworks)' == '' ">net35;net462;net8.0</TargetFrameworks>tests/NLog.Targets.AtomicFile.Tests/ProcessRunner.cs (1)
92-127: Consider capturing or redirecting console output in non-NETFRAMEWORK scenarios.
When running the generatedRunner.dllwithdotnet, you may encounter large output streams if the invoked method prints heavily, potentially filling the console buffer. Provide an option to redirect or drain standard output and error in theSpawnMethodif you expect more extensive logging.src/NLog.Targets.AtomicFile/AtomicFileTarget.cs (1)
64-70: Be mindful of performance impact from a buffer size of 1.
Using a buffer size of 1 ensures atomic appends and minimal in-memory caching. However, for very high throughput use cases, this might degrade performance. Consider making this configurable if needed for different production scenarios.tests/NLog.Targets.AtomicFile.Tests/ConcurrentWritesMultiProcessTests.cs (2)
277-277: Minor spelling correction: “reodered” → “reordered.”
This is a small typo that may confuse future maintainers or hamper searchability.- var reodered = receivedNumbers.OrderBy(i => i); + var reordered = receivedNumbers.OrderBy(i => i);
328-339: Encourage increased log volume or multi-process concurrency.
Your comment suggests that meaningful tests require higher log counts to truly stress concurrency. If needed for deeper validation, consider parameterizing these test loops or adding additional test methods with a larger number of messages and/or processes to capture corner cases such as extended run times.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
build.ps1(1 hunks)run-tests.ps1(2 hunks)src/NLog.Targets.AtomicFile/AtomicFileTarget.cs(1 hunks)src/NLog.Targets.AtomicFile/NLog.Targets.AtomicFile.csproj(1 hunks)src/NLog.sln(3 hunks)tests/NLog.Targets.AtomicFile.Tests/ConcurrentWritesMultiProcessTests.cs(1 hunks)tests/NLog.Targets.AtomicFile.Tests/NLog.Targets.AtomicFile.Tests.csproj(1 hunks)tests/NLog.Targets.AtomicFile.Tests/ProcessRunner.cs(1 hunks)tests/NLog.Targets.AtomicFile.Tests/Properties/AssemblyInfo.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentFileTargetTests.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentWritesMultiProcessTests.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/FileAppenderCacheTests.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/FilePathLayoutTests.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/ProcessRunner.cs(1 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentFileTargetTests.cs (1)
tests/NLog.UnitTests/Layouts/LayoutTypedTests.cs (1)
NLog(1173-1182)
tests/NLog.Targets.AtomicFile.Tests/ProcessRunner.cs (1)
tests/NLog.Targets.ConcurrentFile.Tests/ProcessRunner.cs (3)
ProcessRunner(46-135)ProcessRunner(48-101)Process(103-134)
tests/NLog.Targets.AtomicFile.Tests/ConcurrentWritesMultiProcessTests.cs (3)
tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentWritesMultiProcessTests.cs (10)
ConcurrentWritesMultiProcessTests(53-372)ConcurrentWritesMultiProcessTests(57-62)ConfigureSharedFile(64-110)MultiProcessExecutor(113-170)MakeFileName(172-176)DoConcurrentTest(178-306)Theory(308-335)Theory(337-351)Theory(353-361)Theory(363-371)src/NLog.Targets.AtomicFile/AtomicFileTarget.cs (1)
AtomicFileTarget(41-117)tests/NLog.Targets.ConcurrentFile.Tests/ProcessRunner.cs (3)
Process(103-134)ProcessRunner(46-135)ProcessRunner(48-101)
🔇 Additional comments (18)
tests/NLog.Targets.ConcurrentFile.Tests/FilePathLayoutTests.cs (1)
34-34: Namespace updated to align with new test organization.The namespace has been updated from
NLog.UnitTests.InternaltoNLog.Targets.ConcurrentFile.Tests, which properly aligns this test class with other tests for theConcurrentFiletarget functionality. This change improves organization and discoverability of related tests.build.ps1 (1)
38-38: Adding NLog.Targets.AtomicFile package configuration.This adds the new
NLog.Targets.AtomicFilepackage to the build process with support for .NET 3.5, 4.5, 4.6, and 8.0 frameworks. The inclusion of .NET 8.0 is notable as it shows forward compatibility with the latest .NET version.tests/NLog.Targets.ConcurrentFile.Tests/FileAppenderCacheTests.cs (1)
34-34: Namespace updated to align with new test organization.The namespace has been updated from
NLog.UnitTests.Internal.FileAppenderstoNLog.Targets.ConcurrentFile.Tests, consistent with the refactoring of other test files. This change improves the organization of tests by grouping them by functionality rather than by internal implementation details.tests/NLog.Targets.ConcurrentFile.Tests/ProcessRunner.cs (1)
36-36: Namespace updated to align with new test organization.The namespace has been updated from
NLog.UnitTeststoNLog.Targets.ConcurrentFile.Tests, ensuring consistency with other test files in this functional area. This ProcessRunner utility class is important for multi-process file access testing, so it makes sense to include it with the other concurrent file tests.tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentWritesMultiProcessTests.cs (1)
38-38: Namespace Update ConsistencyThe namespace has been updated to
NLog.Targets.ConcurrentFile.Tests, which aligns with the reorganized test structure. Ensure that all references and documentation match this new namespace.run-tests.ps1 (2)
27-29: LGTM: Test execution added correctly for Windows environment.The test command for the new NLog.Targets.AtomicFile.Tests project is correctly integrated into the Windows test section, following the same pattern as other test projects.
86-89: LGTM: Test execution added correctly for non-Windows environment.The test command specifically targets the net8.0 framework for non-Windows environments, which is consistent with how other test projects are handled in this script.
src/NLog.sln (3)
80-83: LGTM: New projects properly defined.The NLog.Targets.AtomicFile and NLog.Targets.AtomicFile.Tests projects are correctly defined with unique GUIDs and appropriate paths.
194-201: LGTM: Project configurations properly defined.The Debug and Release configurations for both new projects are correctly set up following the same pattern as other projects in the solution.
231-232: LGTM: Projects correctly added to solution structure.Both projects are appropriately added to the "Packages" solution folder, consistent with the organization of other similar projects.
src/NLog.Targets.AtomicFile/NLog.Targets.AtomicFile.csproj (2)
87-89: LGTM: Appropriate use of platform-specific dependency.The reference to Mono.Posix.NETStandard for .NET 8.0 is appropriate for implementing atomic file operations in a cross-platform manner.
91-93: LGTM: Good practice for package resource management.The DownloadMissingContent target ensures the package icon is available during build, making the package distribution more reliable.
tests/NLog.Targets.AtomicFile.Tests/NLog.Targets.AtomicFile.Tests.csproj (1)
20-29: LGTM: Appropriate test framework and dependencies.The xUnit test framework and supporting tools are appropriately configured. The inclusion of Microsoft.CodeAnalysis.CSharp and Basic.Reference.Assemblies suggests runtime code generation capabilities, which is useful for dynamic test scenarios.
tests/NLog.Targets.AtomicFile.Tests/ProcessRunner.cs (2)
48-62: Provide usage disclaimers or handle unexpected arguments more gracefully.
While this code segment correctly validates the number of arguments, there is a potential for user confusion or security concerns if the assembly name, type name, or method name are malformed or malicious in non-test scenarios. Consider clarifying that this utility is intended for trusted test environments, or expand the usage instructions to inform users of expected argument formats.
131-162: Potential compatibility detail with “exec” on Windows shells.
Theexeccommand is generally a Unix shell builtin and might not behave as expected in certain Windows environments if the test harness runs undercmd.exeor PowerShell. Verify that you always execute this code in a Unix-compatible shell environment for non-.NET Framework testing, or introduce an additional conditional step to avoidexecwhen not on Unix-like systems.src/NLog.Targets.AtomicFile/AtomicFileTarget.cs (2)
49-53: Validate concurrency logic fallback.
The code immediately falls back to the base implementation if!ConcurrentWritesor!KeepFileOpen. Confirm that multi-process setups that rely on concurrency are configured properly and do not inadvertently operate in non-atomic mode due to overlooked settings.
58-83: Validate cross-platform correctness for concurrent writes.
When usingFileSystemAclExtensions.Createon Windows or the Unix-specific logic, consider verifying all edge cases with large, frequent writes. In particular, ensure correct handling of file deletion permissions and directory locking during concurrent access.tests/NLog.Targets.AtomicFile.Tests/ConcurrentWritesMultiProcessTests.cs (1)
70-71: Confirm correct enforcement of concurrency settings.
You setKeepFileOpen = falseonly when “retry” is part of themodestring. Confirm that disabling file persistence is the desired approach in these scenarios and that the underlying concurrency logic inAtomicFileTargetis exercised correctly even with the file closed on each write.
83cd06a to
fbc991e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/NLog.Targets.AtomicFile/AtomicFileTarget.cs (1)
49-89: Consider buffering trade-offs in CreateFileStreamUsing
bufferSize: 1ensures minimal internal buffering for atomic append, but it may adversely affect throughput under heavy logging. Confirm whether you need fully unbuffered writes or if a slightly larger buffer would maintain atomicity while improving performance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
build.ps1(1 hunks)run-tests.ps1(2 hunks)src/NLog.Targets.AtomicFile/AtomicFileTarget.cs(1 hunks)src/NLog.Targets.AtomicFile/NLog.Targets.AtomicFile.csproj(1 hunks)src/NLog.sln(3 hunks)src/NLog/Targets/ConsoleTargetHelper.cs(1 hunks)tests/NLog.Targets.AtomicFile.Tests/ConcurrentWritesMultiProcessTests.cs(1 hunks)tests/NLog.Targets.AtomicFile.Tests/NLog.Targets.AtomicFile.Tests.csproj(1 hunks)tests/NLog.Targets.AtomicFile.Tests/ProcessRunner.cs(1 hunks)tests/NLog.Targets.AtomicFile.Tests/Properties/AssemblyInfo.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentFileTargetTests.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentWritesMultiProcessTests.cs(3 hunks)tests/NLog.Targets.ConcurrentFile.Tests/FileAppenderCacheTests.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/FilePathLayoutTests.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/ProcessRunner.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- build.ps1
- tests/NLog.Targets.ConcurrentFile.Tests/FileAppenderCacheTests.cs
- tests/NLog.Targets.AtomicFile.Tests/Properties/AssemblyInfo.cs
- tests/NLog.Targets.ConcurrentFile.Tests/ProcessRunner.cs
- tests/NLog.Targets.ConcurrentFile.Tests/FilePathLayoutTests.cs
- run-tests.ps1
- tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentFileTargetTests.cs
- tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentWritesMultiProcessTests.cs
- src/NLog.sln
- src/NLog.Targets.AtomicFile/NLog.Targets.AtomicFile.csproj
- tests/NLog.Targets.AtomicFile.Tests/NLog.Targets.AtomicFile.Tests.csproj
- tests/NLog.Targets.AtomicFile.Tests/ProcessRunner.cs
- tests/NLog.Targets.AtomicFile.Tests/ConcurrentWritesMultiProcessTests.cs
🧰 Additional context used
🧬 Code Definitions (1)
src/NLog.Targets.AtomicFile/AtomicFileTarget.cs (1)
tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentFileTargetTests.cs (1)
Exists(3397-3400)
🔇 Additional comments (3)
src/NLog/Targets/ConsoleTargetHelper.cs (1)
53-55: Potential regression for non-NETFRAMEWORK runtimesThis new check for Mono availability is now only compiled under
#if NETFRAMEWORK, meaning that on non-NETFRAMEWORK runtimes (e.g., .NET 6/7/8 on Mono), the extra detection logic is skipped. If you intend to support Mono on those newer runtimes, consider replicating or conditionally restoring this check outside theNETFRAMEWORKblock.Would you like me to generate a quick search script to confirm that Mono-based usage outside NETFRAMEWORK is not inadvertently excluded from this code path?
src/NLog.Targets.AtomicFile/AtomicFileTarget.cs (2)
46-46: Confirm default behavior for concurrent loggingDefaulting
ConcurrentWritestotruechanges behavior for existing consumers who may not expect multi-process write-sharing. Verify that this default won't introduce unexpected side effects for users who rely on exclusive file access.
91-115: Validate partial writes or high-volume concurrency scenariosYour Unix-specific system call (
O_APPEND) handles atomic offset updates, but partial writes under high concurrency may still occur. Make sure downstream logic can handle partial writes (e.g., reassembling multi-line entries if needed). If full-line atomicity is critical, consider validating message boundaries in the caller or buffering.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/NLog.Targets.AtomicFile.Tests/ProcessRunner.cs (1)
131-162: Consider adding process output handling.
SpawnMethodstarts the process without capturing or draining standard output, which can cause the application to block if the child process writes a significant amount of data to stdout. For robust test scenarios, consider redirecting and reading the child process’s output.src/NLog.Targets.AtomicFile/AtomicFileTarget.cs (2)
54-71: Potential performance tradeoff with bufferSize=1.
Using buffer size 1 ensures writes go directly to disk for atomic append, but this can impact performance on busy systems. If performance becomes an issue, consider exposing the buffer size as a configurable parameter or implementing a minimal buffering strategy.
95-99: Directory creation concurrency.
When multiple processes simultaneously create the same directory, a race condition could arise. Typically, the OS handles this gracefully, but you may consider catching the resulting exception and ignoring it if the directory is already created by another process.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
build.ps1(1 hunks)run-tests.ps1(2 hunks)src/NLog.Targets.AtomicFile/AtomicFileTarget.cs(1 hunks)src/NLog.Targets.AtomicFile/NLog.Targets.AtomicFile.csproj(1 hunks)src/NLog.sln(3 hunks)src/NLog/Targets/ConsoleTargetHelper.cs(1 hunks)tests/NLog.Targets.AtomicFile.Tests/ConcurrentWritesMultiProcessTests.cs(1 hunks)tests/NLog.Targets.AtomicFile.Tests/NLog.Targets.AtomicFile.Tests.csproj(1 hunks)tests/NLog.Targets.AtomicFile.Tests/ProcessRunner.cs(1 hunks)tests/NLog.Targets.AtomicFile.Tests/Properties/AssemblyInfo.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentFileTargetTests.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentWritesMultiProcessTests.cs(3 hunks)tests/NLog.Targets.ConcurrentFile.Tests/FileAppenderCacheTests.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/FilePathLayoutTests.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/ProcessRunner.cs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/NLog.Targets.AtomicFile.Tests/NLog.Targets.AtomicFile.Tests.csproj
🚧 Files skipped from review as they are similar to previous changes (12)
- src/NLog/Targets/ConsoleTargetHelper.cs
- build.ps1
- tests/NLog.Targets.AtomicFile.Tests/Properties/AssemblyInfo.cs
- tests/NLog.Targets.ConcurrentFile.Tests/FileAppenderCacheTests.cs
- tests/NLog.Targets.ConcurrentFile.Tests/FilePathLayoutTests.cs
- run-tests.ps1
- src/NLog.Targets.AtomicFile/NLog.Targets.AtomicFile.csproj
- src/NLog.sln
- tests/NLog.Targets.ConcurrentFile.Tests/ProcessRunner.cs
- tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentWritesMultiProcessTests.cs
- tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentFileTargetTests.cs
- tests/NLog.Targets.AtomicFile.Tests/ConcurrentWritesMultiProcessTests.cs
🧰 Additional context used
🧬 Code Definitions (2)
src/NLog.Targets.AtomicFile/AtomicFileTarget.cs (1)
tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentFileTargetTests.cs (1)
Exists(3397-3400)
tests/NLog.Targets.AtomicFile.Tests/ProcessRunner.cs (1)
tests/NLog.Targets.ConcurrentFile.Tests/ProcessRunner.cs (3)
ProcessRunner(46-135)ProcessRunner(48-101)Process(103-134)
🔇 Additional comments (7)
tests/NLog.Targets.AtomicFile.Tests/ProcessRunner.cs (2)
1-33: No functional code changes here.
These lines only contain licensing information and carry no functional or structural modifications to review.
48-129: Validate reference assemblies and runtime compatibility.
When compiling for .NET (non-Framework), the code referencesBasic.Reference.Assemblies.ReferenceAssemblies.Net80and assumes availability of .NET 8.0 references. Please verify that the build environment supports these references for .NET 8.x and consider providing fallback for older target frameworks if needed.src/NLog.Targets.AtomicFile/AtomicFileTarget.cs (5)
1-33: No functional code changes here.
These lines only contain licensing information and code comments, thus no direct changes to review.
34-46: Good addition for multi-process concurrency.
ExposingConcurrentWritesas a property contributes to clarity and configurability. The defaulttrueis sensible when atomic writes are needed for multi-process scenarios.
48-53: Graceful fallback for single-process usage.
The condition that callsbase.CreateFileStreamwhen concurrent writes or keep-file-open is disabled ensures backward compatibility and performance in scenarios without concurrency demands.
72-88: Cross-platform Windows handling looks correct.
LeveragingFileSystemAclExtensions.Createfor atomic append is consistent with the intended concurrency. The code is well-structured to handle the platform distinction.
91-116: Robust Unix implementation of O_APPEND.
The usage ofMono.Posix.Native.Syscall.openwithO_APPENDensures atomic writes, which is vital for multi-process concurrency. The error handling and fallback to directory creation appear solid for typical usage.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/NLog.Targets.AtomicFile/AtomicFileTarget.cs (3)
1-1: Consider removing the BOM (Byte Order Mark).
The BOM at the beginning of the file can sometimes cause issues with certain tools or encodings. Unless there's a specific need for it, removing it may help avoid encoding-related confusion.
49-89: Consider additional safety checks for write errors.
While settingFileMode.Appendwith theSYNCHRONIZEflag on Windows ensures atomic appends, there’s still a potential for errors like disk-full conditions or unexpected I/O exceptions. For more robust resilience, you might handle and log these exceptions explicitly, or implement a retry mechanism if such scenarios are critical.
91-115: Address concurrent directory creation race conditions.
If multiple processes attempt to create the directory at the same time, a race condition might occur.Directory.CreateDirectoryis usually idempotent, but explicitly handling errors from simultaneous directory creation could improve stability under heavy load.If you’d like, I can propose a more defensive pattern that gratifies concurrency attempts when creating directories.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
build.ps1(1 hunks)run-tests.ps1(2 hunks)src/NLog.Targets.AtomicFile/AtomicFileTarget.cs(1 hunks)src/NLog.Targets.AtomicFile/NLog.Targets.AtomicFile.csproj(1 hunks)src/NLog.sln(3 hunks)src/NLog/Targets/ConsoleTargetHelper.cs(1 hunks)tests/NLog.Targets.AtomicFile.Tests/ConcurrentWritesMultiProcessTests.cs(1 hunks)tests/NLog.Targets.AtomicFile.Tests/NLog.Targets.AtomicFile.Tests.csproj(1 hunks)tests/NLog.Targets.AtomicFile.Tests/ProcessRunner.cs(1 hunks)tests/NLog.Targets.AtomicFile.Tests/Properties/AssemblyInfo.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentFileTargetTests.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentWritesMultiProcessTests.cs(3 hunks)tests/NLog.Targets.ConcurrentFile.Tests/FileAppenderCacheTests.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/FilePathLayoutTests.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/ProcessRunner.cs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/NLog.Targets.AtomicFile.Tests/NLog.Targets.AtomicFile.Tests.csproj
🚧 Files skipped from review as they are similar to previous changes (13)
- tests/NLog.Targets.ConcurrentFile.Tests/FilePathLayoutTests.cs
- tests/NLog.Targets.ConcurrentFile.Tests/FileAppenderCacheTests.cs
- build.ps1
- tests/NLog.Targets.AtomicFile.Tests/Properties/AssemblyInfo.cs
- src/NLog/Targets/ConsoleTargetHelper.cs
- run-tests.ps1
- tests/NLog.Targets.ConcurrentFile.Tests/ProcessRunner.cs
- tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentFileTargetTests.cs
- tests/NLog.Targets.AtomicFile.Tests/ProcessRunner.cs
- src/NLog.sln
- tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentWritesMultiProcessTests.cs
- src/NLog.Targets.AtomicFile/NLog.Targets.AtomicFile.csproj
- tests/NLog.Targets.AtomicFile.Tests/ConcurrentWritesMultiProcessTests.cs
🧰 Additional context used
🧬 Code Definitions (1)
src/NLog.Targets.AtomicFile/AtomicFileTarget.cs (1)
tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentFileTargetTests.cs (1)
Exists(3397-3400)
🔇 Additional comments (1)
src/NLog.Targets.AtomicFile/AtomicFileTarget.cs (1)
41-46: Revisit the default concurrency setting.
EnablingConcurrentWritesby default represents a breaking change in behavior compared to a standardFileTarget. Existing setups expecting exclusive writes may be impacted if they upgrade without awareness.Please confirm whether you’ve evaluated all downstream code to ensure it’s not depending on exclusive access, or if you want to announce this as a behavioral change in the release notes.
db668ac to
40319fc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/NLog.Targets.AtomicFile/AtomicFileTarget.cs (4)
38-43: Class documentation should clarify the differences between this target and the standard FileTarget.The class documentation provides a brief overview, but it would be helpful to expand on when to use this target over the standard FileTarget, and what specific atomicity guarantees it provides. Also, consider explaining why there are two target names ("AtomFile" and "AtomicFile").
45-48: Property documentation could be more precise.The documentation for the
ConcurrentWritesproperty has a grammatical issue. Also, it would be helpful to explain the consequences of setting this to false and how it differs from the standard FileTarget behavior./// <summary> -/// Gets or sets a value indicating whether concurrent writes to the log file by multiple processes on the same host. +/// Gets or sets a value indicating whether to allow concurrent writes to the log file by multiple processes on the same host. /// </summary>
94-107: File permissions in Unix implementation are too permissive.The current implementation uses permissions equivalent to 0666 (read/write for owner, group, and others), which may be too permissive in some environments. Consider using more restrictive permissions or making them configurable.
-int fd = Mono.Unix.Native.Syscall.open(filePath, Mono.Unix.Native.OpenFlags.O_CREAT | Mono.Unix.Native.OpenFlags.O_WRONLY | Mono.Unix.Native.OpenFlags.O_APPEND, (Mono.Unix.Native.FilePermissions)(6 | (6 << 3) | (6 << 6))); +// Use 0644 permissions (read/write for owner, read for group and others) +int fd = Mono.Unix.Native.Syscall.open(filePath, Mono.Unix.Native.OpenFlags.O_CREAT | Mono.Unix.Native.OpenFlags.O_WRONLY | Mono.Unix.Native.OpenFlags.O_APPEND, (Mono.Unix.Native.FilePermissions)(6 | (4 << 3) | (4 << 6)));Same change would be needed on line 103.
105-106: Error handling could be more descriptive.When the file descriptor is -1, an exception is thrown, but no additional context is provided. Consider adding a more descriptive error message.
if (fd == -1) - Mono.Unix.UnixMarshal.ThrowExceptionForLastError(); + Mono.Unix.UnixMarshal.ThrowExceptionForLastError($"Failed to open file '{filePath}' for atomic writing");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
build.ps1(3 hunks)run-tests.ps1(2 hunks)src/NLog.Targets.AtomicFile/AtomicFileTarget.cs(1 hunks)src/NLog.Targets.AtomicFile/NLog.Targets.AtomicFile.csproj(1 hunks)src/NLog.sln(3 hunks)src/NLog/Targets/ConsoleTargetHelper.cs(1 hunks)tests/NLog.Targets.AtomicFile.Tests/ConcurrentWritesMultiProcessTests.cs(1 hunks)tests/NLog.Targets.AtomicFile.Tests/NLog.Targets.AtomicFile.Tests.csproj(1 hunks)tests/NLog.Targets.AtomicFile.Tests/ProcessRunner.cs(1 hunks)tests/NLog.Targets.AtomicFile.Tests/Properties/AssemblyInfo.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentFileTargetTests.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentWritesMultiProcessTests.cs(3 hunks)tests/NLog.Targets.ConcurrentFile.Tests/FileAppenderCacheTests.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/FilePathLayoutTests.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/ProcessRunner.cs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/NLog.Targets.AtomicFile.Tests/NLog.Targets.AtomicFile.Tests.csproj
🚧 Files skipped from review as they are similar to previous changes (13)
- tests/NLog.Targets.ConcurrentFile.Tests/FilePathLayoutTests.cs
- src/NLog/Targets/ConsoleTargetHelper.cs
- tests/NLog.Targets.AtomicFile.Tests/Properties/AssemblyInfo.cs
- tests/NLog.Targets.ConcurrentFile.Tests/ProcessRunner.cs
- build.ps1
- tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentFileTargetTests.cs
- src/NLog.Targets.AtomicFile/NLog.Targets.AtomicFile.csproj
- src/NLog.sln
- tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentWritesMultiProcessTests.cs
- tests/NLog.Targets.AtomicFile.Tests/ConcurrentWritesMultiProcessTests.cs
- tests/NLog.Targets.AtomicFile.Tests/ProcessRunner.cs
- tests/NLog.Targets.ConcurrentFile.Tests/FileAppenderCacheTests.cs
- run-tests.ps1
🧰 Additional context used
🧬 Code Definitions (1)
src/NLog.Targets.AtomicFile/AtomicFileTarget.cs (1)
tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentFileTargetTests.cs (1)
Exists(3397-3400)
🔇 Additional comments (4)
src/NLog.Targets.AtomicFile/AtomicFileTarget.cs (4)
56-58: Good handling of file sharing options.The code correctly configures file sharing options to allow read and write access, and conditionally enables delete access based on the
EnableFileDeleteproperty.
60-73: Well-implemented platform-specific approach for .NET Framework.The implementation for .NET Framework correctly uses
FileSystemRights.AppendDataandFileSystemRights.Synchronizeto ensure atomic appending. The comments explaining the behavior are helpful.The small buffer size of 1 is intentional for direct writing, but you might want to add a comment about potential performance implications for large writes.
74-90: Well-structured platform detection for non-.NET Framework environments.Good use of
RuntimeInformation.IsOSPlatformto detect Windows and apply the appropriate implementation.
108-116: Good exception handling and resource cleanup.The code properly handles exceptions and ensures that the file descriptor is closed if an exception occurs during stream creation.
f26e33f to
995a236
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
src/NLog.Targets.AtomicFile/AtomicFileTarget.cs (2)
46-48: Improve property documentation.The documentation for ConcurrentWrites property seems incomplete. It should clearly state what the property does and its default value.
/// <summary> - /// Gets or sets a value indicating whether concurrent writes to the log file by multiple processes on the same host. + /// Gets or sets a value indicating whether concurrent writes to the log file by multiple processes on the same host are supported. + /// When set to true (default), the file is opened in a way that allows multiple processes to write to it simultaneously. /// </summary>
36-37: Consider adding more using statements.Since you're using various file-related types, consider adding explicit using statements for the namespaces that contain them.
using System.IO; + using System.IO.FileSystem; + #if !NETFRAMEWORK + using System.Runtime.InteropServices; + #endiftests/NLog.Targets.AtomicFile.Tests/ProcessRunner.cs (4)
54-90: Consider adding more verbose error output for debuggingThe Main method of the compiled code correctly handles the execution flow but could be enhanced for better debugging in test scenarios.
public static int Main(string[] args) { try { Console.WriteLine("Starting..."); + Console.WriteLine($"Arguments: {string.Join(", ", args)}"); if (args.Length < 3) throw new Exception("Usage: Runner.exe \"AssemblyName\" \"ClassName\" \"MethodName\" \"Parameter1...N\""); string assemblyName = args[0]; string className = args[1]; string methodName = args[2]; object[] arguments = new object[args.Length - 3]; for (int i = 0; i < arguments.Length; ++i) arguments[i] = args[3 + i]; + Console.WriteLine($"Attempting to load assembly: {assemblyName}"); Assembly assembly = Assembly.Load(assemblyName); + Console.WriteLine($"Looking for type: {className}"); Type type = assembly.GetType(className); if (type == null) throw new Exception(className + " not found in " + assemblyName); + Console.WriteLine($"Looking for method: {methodName}"); MethodInfo method = type.GetMethod(methodName); if (method == null) throw new Exception(methodName + " not found in " + type); object targetObject = null; if (!method.IsStatic) targetObject = Activator.CreateInstance(type); + Console.WriteLine("Invoking method..."); method.Invoke(targetObject, arguments); + Console.WriteLine("Method execution completed successfully"); return 0; } catch (Exception ex) { Console.WriteLine(ex); return 1; } }
113-113: Remove commented out codeThe commented line using MemoryStream should be removed if not needed.
- //using (var stream = new MemoryStream())
107-107: Consider making the target framework reference configurableHardcoding
NetStandard20references might cause issues if targeting different frameworks in the future.-references: Basic.Reference.Assemblies.ReferenceAssemblies.NetStandard20); +var targetFramework = GetTargetFrameworkReferences(); +references: targetFramework); // Add this helper method to the class: +private static Microsoft.CodeAnalysis.MetadataReference[] GetTargetFrameworkReferences() +{ + // You can expand this method to detect the current runtime and return appropriate references + return Basic.Reference.Assemblies.ReferenceAssemblies.NetStandard20; +}
155-161: Consider adding stdout/stderr redirection optionThe comment about redirecting stdout is helpful, but you might want to add an option to enable this in some test scenarios.
public static Process SpawnMethod(Type type, string methodName, params string[] p) +{ + return SpawnMethod(type, methodName, false, p); +} + +public static Process SpawnMethod(Type type, string methodName, bool redirectOutput, params string[] p) { string assemblyName = type.Assembly.FullName; string typename = type.FullName; StringBuilder sb = new StringBuilder(); #if !NETFRAMEWORK sb.AppendFormat("exec \"{0}\" ", Path.Combine(AppDomain.CurrentDomain.BaseDirectory, "Runner.dll")); #endif sb.AppendFormat("\"{0}\" \"{1}\" \"{2}\"", assemblyName, typename, methodName); foreach (string s in p) { sb.Append(" "); sb.Append("\""); sb.Append(s); sb.Append("\""); } Process proc = new Process(); proc.StartInfo.Arguments = sb.ToString(); #if NETFRAMEWORK proc.StartInfo.FileName = Path.Combine(AppDomain.CurrentDomain.BaseDirectory, "Runner.exe"); #else proc.StartInfo.FileName = "dotnet"; #endif proc.StartInfo.UseShellExecute = false; proc.StartInfo.CreateNoWindow = true; + + if (redirectOutput) + { + proc.StartInfo.RedirectStandardOutput = true; + proc.StartInfo.RedirectStandardError = true; + proc.OutputDataReceived += (sender, e) => { if (e.Data != null) Console.WriteLine($"[Runner Output] {e.Data}"); }; + proc.ErrorDataReceived += (sender, e) => { if (e.Data != null) Console.WriteLine($"[Runner Error] {e.Data}"); }; + } // Hint: // In case we wanna redirect stdout we should drain the redirected pipe continuously. // Otherwise Runner.exe's console buffer is full rather fast, leading to a lock within Console.Write(Line). proc.Start(); + + if (redirectOutput) + { + proc.BeginOutputReadLine(); + proc.BeginErrorReadLine(); + } return proc; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
build.ps1(2 hunks)run-tests.ps1(2 hunks)src/NLog.Targets.AtomicFile/AtomicFileTarget.cs(1 hunks)src/NLog.Targets.AtomicFile/NLog.Targets.AtomicFile.csproj(1 hunks)src/NLog.sln(3 hunks)src/NLog/Config/ConfigurationItemFactory.cs(1 hunks)src/NLog/Targets/ConsoleTargetHelper.cs(1 hunks)tests/NLog.Targets.AtomicFile.Tests/ConcurrentWritesMultiProcessTests.cs(1 hunks)tests/NLog.Targets.AtomicFile.Tests/NLog.Targets.AtomicFile.Tests.csproj(1 hunks)tests/NLog.Targets.AtomicFile.Tests/ProcessRunner.cs(1 hunks)tests/NLog.Targets.AtomicFile.Tests/Properties/AssemblyInfo.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentFileTargetTests.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentWritesMultiProcessTests.cs(3 hunks)tests/NLog.Targets.ConcurrentFile.Tests/FileAppenderCacheTests.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/FilePathLayoutTests.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/ProcessRunner.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- tests/NLog.Targets.ConcurrentFile.Tests/FilePathLayoutTests.cs
- src/NLog/Targets/ConsoleTargetHelper.cs
- tests/NLog.Targets.ConcurrentFile.Tests/ProcessRunner.cs
- tests/NLog.Targets.AtomicFile.Tests/Properties/AssemblyInfo.cs
- tests/NLog.Targets.ConcurrentFile.Tests/FileAppenderCacheTests.cs
- build.ps1
- run-tests.ps1
- tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentFileTargetTests.cs
- tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentWritesMultiProcessTests.cs
- src/NLog.sln
- tests/NLog.Targets.AtomicFile.Tests/NLog.Targets.AtomicFile.Tests.csproj
- src/NLog.Targets.AtomicFile/NLog.Targets.AtomicFile.csproj
- tests/NLog.Targets.AtomicFile.Tests/ConcurrentWritesMultiProcessTests.cs
🧰 Additional context used
🧬 Code Definitions (1)
tests/NLog.Targets.AtomicFile.Tests/ProcessRunner.cs (1)
tests/NLog.Targets.ConcurrentFile.Tests/ProcessRunner.cs (3)
ProcessRunner(46-135)ProcessRunner(48-101)Process(103-134)
🔇 Additional comments (7)
src/NLog.Targets.AtomicFile/AtomicFileTarget.cs (3)
41-43: LGTM! Good use of multiple target attributes.The implementation correctly defines two target attributes ("AtomFile" and "AtomicFile") to allow flexibility in configuration.
51-90: LGTM! Well-implemented platform-specific file stream creation.The implementation correctly handles different platforms:
- For .NET Framework: Uses FileStream with AppendData | Synchronize rights
- For Windows on non-.NET Framework: Uses FileSystemAclExtensions.Create
- For Unix: Uses Mono.Unix.Native syscalls with O_APPEND flag
The bufferSize of 1 ensures immediate writes without buffering, which is important for atomic operations.
94-122: LGTM! Good error handling in Unix stream creation.The implementation correctly:
- Creates directories if needed and configured
- Properly handles file open errors
- Ensures file descriptor cleanup in case of exceptions
The use of O_APPEND flag ensures atomic writes at the OS level on Unix systems.
tests/NLog.Targets.AtomicFile.Tests/ProcessRunner.cs (4)
1-33: LGTM: License header is appropriately includedThe file includes the standard NLog license header which is consistent with other files in the project.
36-45: LGTM: Using statements are appropriateAll necessary namespaces are included for the functionality, including Microsoft.CodeAnalysis.CSharp which is needed for the Roslyn compilation used in non-.NET Framework environments.
136-138: LGTM: Proper command formatting for different frameworksThe command construction appropriately uses "exec" for non-Framework targets to ensure the shell process is replaced by the dotnet process.
150-154: LGTM: Framework-specific process executionThe implementation correctly handles the different ways to execute the compiled code based on the target framework.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/NLog.Targets.AtomicFile.Tests/ProcessRunner.cs (2)
100-100: 🛠️ Refactor suggestionAdd a null check for the TEMP environment variable.
The current implementation could throw an exception if the TEMP environment variable is not set.
-options.TempFiles = new TempFileCollection(Environment.GetEnvironmentVariable("TEMP"), true); +var tempPath = Environment.GetEnvironmentVariable("TEMP") ?? Path.GetTempPath(); +options.TempFiles = new TempFileCollection(tempPath, true);
123-127: 🛠️ Refactor suggestionAdd error handling for missing runtimeconfig.json.
The code should check if the source runtimeconfig.json file exists before attempting to copy it.
-using (var streamReader = new StreamReader(Path.Combine(Path.GetDirectoryName(typeof(ProcessRunner).Assembly.Location), typeof(ProcessRunner).Assembly.GetName().Name) + ".runtimeconfig.json")) -using (var streamWriter = new StreamWriter(outputRuntimeConfig)) -{ - streamWriter.Write(streamReader.ReadToEnd()); -} +var sourceRuntimeConfig = Path.Combine(Path.GetDirectoryName(typeof(ProcessRunner).Assembly.Location), typeof(ProcessRunner).Assembly.GetName().Name + ".runtimeconfig.json"); +if (System.IO.File.Exists(sourceRuntimeConfig)) +{ + using (var streamReader = new StreamReader(sourceRuntimeConfig)) + using (var streamWriter = new StreamWriter(outputRuntimeConfig)) + { + streamWriter.Write(streamReader.ReadToEnd()); + } +} +else +{ + // Create a minimal runtimeconfig.json if source doesn't exist + using (var streamWriter = new StreamWriter(outputRuntimeConfig)) + { + streamWriter.Write("{ \"runtimeOptions\": { \"tfm\": \"netcoreapp3.1\", \"framework\": { \"name\": \"Microsoft.NETCore.App\", \"version\": \"3.1.0\" } } }"); + } +}
🧹 Nitpick comments (7)
src/NLog.Targets.AtomicFile/AtomicFileTarget.cs (3)
45-48: Improve the property documentation clarity.The description for the
ConcurrentWritesproperty is grammatically incomplete. It appears to be missing a verb or additional phrasing.- /// Gets or sets a value indicating whether concurrent writes to the log file by multiple processes on the same host. + /// Gets or sets a value indicating whether to enable concurrent writes to the log file by multiple processes on the same host.
71-72: Consider documenting the performance implications of buffer size 1.Setting the buffer size to 1 disables internal buffering which ensures immediate writes but may impact performance. Consider adding a comment explaining this trade-off for maintainability.
97-97: Clarify the file permission calculation.The permissions calculation is correct but difficult to read. Consider using octal notation or named constants for better clarity.
- var permissions = (Mono.Unix.Native.FilePermissions)(6 | (6 << 3) | (6 << 6)); + // Owner: read/write (6), Group: read (4), Others: read (4) = 0644 + var permissions = (Mono.Unix.Native.FilePermissions)(6 << 6 | 4 << 3 | 4);tests/NLog.Targets.AtomicFile.Tests/ProcessRunner.cs (4)
113-113: Remove commented-out code.The commented line appears to be leftover development code and should be removed for clarity.
- //using (var stream = new MemoryStream())
54-54: Use internal class to avoid potential naming conflicts.The public class
C1could potentially clash with other classes in loaded assemblies. Consider using a more specific name or making it internal.-public static class C1 +internal static class RunnerProgram
156-161: Consider adding process output redirection.For better diagnostics, consider adding options to redirect and capture the process output, especially since you're already handling console output in the runner program.
proc.StartInfo.UseShellExecute = false; proc.StartInfo.CreateNoWindow = true; // Hint: // In case we wanna redirect stdout we should drain the redirected pipe continuously. // Otherwise Runner.exe's console buffer is full rather fast, leading to a lock within Console.Write(Line). +proc.StartInfo.RedirectStandardOutput = true; +proc.StartInfo.RedirectStandardError = true; +var outputBuilder = new StringBuilder(); +var errorBuilder = new StringBuilder(); +proc.OutputDataReceived += (sender, args) => { if (args.Data != null) outputBuilder.AppendLine(args.Data); }; +proc.ErrorDataReceived += (sender, args) => { if (args.Data != null) errorBuilder.AppendLine(args.Data); }; proc.Start(); +proc.BeginOutputReadLine(); +proc.BeginErrorReadLine();
92-129: Add compiler error details for NETFRAMEWORK similar to .NET Core path.The NETFRAMEWORK code path doesn't provide detailed compiler error information when compilation fails, unlike the .NET Core path which displays detailed diagnostics.
#if NETFRAMEWORK CSharpCodeProvider provider = new CSharpCodeProvider(); var options = new CompilerParameters(); options.OutputAssembly = "Runner.exe"; options.GenerateExecutable = true; options.IncludeDebugInformation = true; // To allow debugging the generated Runner.exe we need to keep files. // See https://stackoverflow.com/questions/875723/how-to-debug-break-in-codedom-compiled-code var tempPath = Environment.GetEnvironmentVariable("TEMP") ?? Path.GetTempPath(); options.TempFiles = new TempFileCollection(tempPath, true); var results = provider.CompileAssemblyFromSource(options, sourceCode); - Assert.False(results.Errors.HasWarnings); - Assert.False(results.Errors.HasErrors); + if (results.Errors.HasWarnings || results.Errors.HasErrors) + { + var errorMessages = new StringBuilder(); + foreach (CompilerError error in results.Errors) + { + errorMessages.AppendLine($"{error.ErrorNumber}: {error.ErrorText} at line {error.Line}"); + } + Assert.False(true, errorMessages.ToString()); + } #else
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
build.ps1(2 hunks)run-tests.ps1(2 hunks)src/NLog.Targets.AtomicFile/AtomicFileTarget.cs(1 hunks)src/NLog.Targets.AtomicFile/NLog.Targets.AtomicFile.csproj(1 hunks)src/NLog.sln(3 hunks)src/NLog/Config/ConfigurationItemFactory.cs(1 hunks)src/NLog/Targets/ConsoleTargetHelper.cs(1 hunks)tests/NLog.Targets.AtomicFile.Tests/ConcurrentWritesMultiProcessTests.cs(1 hunks)tests/NLog.Targets.AtomicFile.Tests/NLog.Targets.AtomicFile.Tests.csproj(1 hunks)tests/NLog.Targets.AtomicFile.Tests/ProcessRunner.cs(1 hunks)tests/NLog.Targets.AtomicFile.Tests/Properties/AssemblyInfo.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentFileTargetTests.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentWritesMultiProcessTests.cs(3 hunks)tests/NLog.Targets.ConcurrentFile.Tests/FileAppenderCacheTests.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/FilePathLayoutTests.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/ProcessRunner.cs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/NLog.Targets.AtomicFile.Tests/NLog.Targets.AtomicFile.Tests.csproj
🚧 Files skipped from review as they are similar to previous changes (13)
- tests/NLog.Targets.ConcurrentFile.Tests/ProcessRunner.cs
- src/NLog/Targets/ConsoleTargetHelper.cs
- tests/NLog.Targets.AtomicFile.Tests/Properties/AssemblyInfo.cs
- src/NLog/Config/ConfigurationItemFactory.cs
- run-tests.ps1
- build.ps1
- tests/NLog.Targets.ConcurrentFile.Tests/FilePathLayoutTests.cs
- tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentFileTargetTests.cs
- src/NLog.sln
- tests/NLog.Targets.ConcurrentFile.Tests/FileAppenderCacheTests.cs
- tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentWritesMultiProcessTests.cs
- src/NLog.Targets.AtomicFile/NLog.Targets.AtomicFile.csproj
- tests/NLog.Targets.AtomicFile.Tests/ConcurrentWritesMultiProcessTests.cs
🧰 Additional context used
🧬 Code Definitions (1)
tests/NLog.Targets.AtomicFile.Tests/ProcessRunner.cs (1)
tests/NLog.Targets.ConcurrentFile.Tests/ProcessRunner.cs (3)
ProcessRunner(46-135)ProcessRunner(48-101)Process(103-134)
🔇 Additional comments (4)
src/NLog.Targets.AtomicFile/AtomicFileTarget.cs (4)
38-43: Good use of both target names and clear class documentation.The class provides descriptive XML documentation and uses both "AtomFile" and "AtomicFile" target names, making it flexible for configuration. The implementation correctly extends FileTarget to add atomic file appending capabilities.
50-91: Well-implemented platform-specific file stream creation.The implementation correctly handles different platforms and frameworks with appropriate checks. The method:
- Returns early for non-concurrent writes or non-persistent file handles
- Properly configures file sharing options
- Uses platform-specific atomic append mechanisms
The use of
FileSystemRights.AppendData | FileSystemRights.Synchronizeensures atomic appends on Windows, which directly addresses the PR objective.
93-122: Comprehensive Unix implementation with proper error handling.The Unix implementation properly:
- Uses O_APPEND flag for atomic appends (addressing the PR objective)
- Handles directory creation if needed
- Sets appropriate file permissions
- Manages resources even in error cases
One suggestion would be to ensure the implementation doesn't have unintended file permission issues.
The permissions are set to 0644 (Owner: read/write, Group: read, Others: read). Please verify this aligns with your security requirements for log files, as some environments might require more restrictive permissions.
1-124: Overall implementation successfully addresses the PR objective.The implementation successfully adds support for O_APPEND on Linux through
Mono.Posix.NETStandardas stated in the PR objectives, while also providing equivalent functionality for Windows platforms. The class is well-structured with appropriate platform checks and error handling.A few items to consider:
- Adding unit tests specifically for the Unix implementation
- Documenting the performance characteristics of atomic appending
- Ensuring appropriate runtime checks for the Mono.Posix dependency
These are not blocking issues but would improve the implementation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/NLog/Config/ConfigurationItemFactory.cs (2)
432-461: Consider consolidating name registrations in a loop for maintainability.Currently, each unique target name is manually passed to
SafeRegisterNamedType. If you need to add or remove multiple targets, you will have to repeat or remove lines manually, which can lead to accidental omissions. Consider using a data structure (e.g., an array or list of tuples) to hold the(typeAlias, fullTypeName)pairs, then loop over them. This makes the code more maintainable and reduces duplication.
463-469: Validate type alias before registration.Although
_targets.CheckTypeAliasExistshelps avoid duplicate registrations, consider validating that thetypeAliasandfullTypeNameare non-empty strings. This prevents potential runtime errors if an empty or null alias is inadvertently passed.src/NLog.Targets.AtomicFile/AtomicFileTarget.cs (1)
45-48: Optional: Document concurrency effect.While the
ConcurrentWritesproperty is self-explanatory, consider adding a concise XML doc comment explaining that ifConcurrentWritesistrue, the target attempts atomic append using OS-specific mechanisms, and iffalse, it reverts to classic file handling. This helps maintainers understand the concurrency model better.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
build.ps1(1 hunks)run-tests.ps1(2 hunks)src/NLog.Targets.AtomicFile/AtomicFileTarget.cs(1 hunks)src/NLog.Targets.AtomicFile/NLog.Targets.AtomicFile.csproj(1 hunks)src/NLog.Targets.Network/Layouts/Log4JXmlEventLayout.cs(1 hunks)src/NLog.sln(3 hunks)src/NLog/Config/ConfigurationItemFactory.cs(1 hunks)src/NLog/Targets/ConsoleTargetHelper.cs(1 hunks)tests/NLog.Targets.AtomicFile.Tests/ConcurrentWritesMultiProcessTests.cs(1 hunks)tests/NLog.Targets.AtomicFile.Tests/NLog.Targets.AtomicFile.Tests.csproj(1 hunks)tests/NLog.Targets.AtomicFile.Tests/ProcessRunner.cs(1 hunks)tests/NLog.Targets.AtomicFile.Tests/Properties/AssemblyInfo.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentFileTargetTests.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentWritesMultiProcessTests.cs(3 hunks)tests/NLog.Targets.ConcurrentFile.Tests/FileAppenderCacheTests.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/FilePathLayoutTests.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/ProcessRunner.cs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/NLog.Targets.AtomicFile.Tests/NLog.Targets.AtomicFile.Tests.csproj
🚧 Files skipped from review as they are similar to previous changes (13)
- tests/NLog.Targets.ConcurrentFile.Tests/FilePathLayoutTests.cs
- tests/NLog.Targets.AtomicFile.Tests/Properties/AssemblyInfo.cs
- src/NLog/Targets/ConsoleTargetHelper.cs
- tests/NLog.Targets.ConcurrentFile.Tests/FileAppenderCacheTests.cs
- tests/NLog.Targets.ConcurrentFile.Tests/ProcessRunner.cs
- run-tests.ps1
- build.ps1
- src/NLog.Targets.AtomicFile/NLog.Targets.AtomicFile.csproj
- tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentFileTargetTests.cs
- src/NLog.sln
- tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentWritesMultiProcessTests.cs
- tests/NLog.Targets.AtomicFile.Tests/ProcessRunner.cs
- tests/NLog.Targets.AtomicFile.Tests/ConcurrentWritesMultiProcessTests.cs
🧰 Additional context used
🧬 Code Definitions (1)
src/NLog.Targets.Network/Layouts/Log4JXmlEventLayout.cs (1)
src/NLog.Targets.Network/Layouts/SyslogLayout.cs (1)
Layout(14-520)
🔇 Additional comments (7)
src/NLog.Targets.AtomicFile/AtomicFileTarget.cs (6)
1-33: No issues with license header.
34-43: Class naming alignment.The class attributes
[Target("AtomFile")]and[Target("AtomicFile")]are consistent with the type's name, making it easy for users to reference. No issues found here.
50-55: Correct fallback approach.When
ConcurrentWritesis false or whenKeepFileOpenis false, the code delegates to the baseCreateFileStream, which is the right approach for standard file handling. No changes needed.
56-73: NET Framework atomic append usage is valid.Using
FileMode.Appendtogether withFileSystemRights.AppendData | FileSystemRights.Synchronizeis a standard atomic append approach on Windows with .NET Framework. The approach aligns well with published guidelines on concurrency for file appending.
74-91: Recommendation: Ensure fallback for unsupported OS versions.For Windows on .NET (non-Framework), the usage of
FileSystemAclExtensions.Createis correct for supporting atomic append. Consider verifying that your build configuration ensures references to the correct platform assembly. If users run on older Windows versions or special environments without ACL support, confirm that an appropriate fallback or clear exception is provided.Would you like a script to cross-check references to
FileSystemAclExtensionsin the codebase and confirm all are covered by conditional compilation?
93-121: Graceful directory creation and error handling.The implementation of
CreateUnixStreamusesSyscall.openwithO_APPENDand retrying directory creation ifENOENTis encountered. This is a good pattern that gracefully handles missing parent directories. Checking for last error and callingThrowExceptionForLastErrorensures robust error reporting.src/NLog.Targets.Network/Layouts/Log4JXmlEventLayout.cs (1)
54-54: Additional layout alias increases flexibility.The extra
[Layout("Log4JXmlLayout")]attribute makes referencing simpler. This change poses no conflicts and aligns with the practice used in other layout classes (e.g., SyslogLayout).
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
tests/NLog.Targets.AtomicFile.Tests/ProcessRunner.cs (3)
100-100:⚠️ Potential issueAdd null check for TEMP environment variable.
The current implementation could throw an exception if the TEMP environment variable is not set.
-options.TempFiles = new TempFileCollection(Environment.GetEnvironmentVariable("TEMP"), true); +var tempPath = Environment.GetEnvironmentVariable("TEMP") ?? Path.GetTempPath(); +options.TempFiles = new TempFileCollection(tempPath, true);
120-127:⚠️ Potential issueAdd error handling for missing runtimeconfig.json.
The code should check if the source runtimeconfig.json file exists before attempting to copy it.
var outputRuntimeConfig = Path.Combine(Path.GetDirectoryName(typeof(ProcessRunner).Assembly.Location), "Runner.runtimeconfig.json"); if (System.IO.File.Exists(outputRuntimeConfig)) System.IO.File.Delete(outputRuntimeConfig); -using (var streamReader = new StreamReader(Path.Combine(Path.GetDirectoryName(typeof(ProcessRunner).Assembly.Location), typeof(ProcessRunner).Assembly.GetName().Name) + ".runtimeconfig.json")) -using (var streamWriter = new StreamWriter(outputRuntimeConfig)) -{ - streamWriter.Write(streamReader.ReadToEnd()); -} +var sourceRuntimeConfig = Path.Combine(Path.GetDirectoryName(typeof(ProcessRunner).Assembly.Location), typeof(ProcessRunner).Assembly.GetName().Name + ".runtimeconfig.json"); +if (System.IO.File.Exists(sourceRuntimeConfig)) +{ + using (var streamReader = new StreamReader(sourceRuntimeConfig)) + using (var streamWriter = new StreamWriter(outputRuntimeConfig)) + { + streamWriter.Write(streamReader.ReadToEnd()); + } +} +else +{ + // Create a minimal runtimeconfig.json if source doesn't exist + using (var streamWriter = new StreamWriter(outputRuntimeConfig)) + { + streamWriter.Write("{ \"runtimeOptions\": { \"tfm\": \"netcoreapp3.1\", \"framework\": { \"name\": \"Microsoft.NETCore.App\", \"version\": \"3.1.0\" } } }"); + } +}
131-162: 🛠️ Refactor suggestionAdd process resource cleanup method.
The
SpawnMethodreturns a Process object but doesn't ensure it gets properly cleaned up, which could lead to resource leaks. Add a utility method to safely dispose of the process.public static Process SpawnMethod(Type type, string methodName, params string[] p) { // existing code... proc.Start(); return proc; } +/// <summary> +/// Safely waits for a process to complete and releases resources +/// </summary> +/// <param name="process">The process to wait for</param> +/// <param name="timeoutMs">Timeout in milliseconds, defaults to 30 seconds</param> +/// <returns>True if process completed, false if it timed out</returns> +public static bool WaitAndCleanup(Process process, int timeoutMs = 30000) +{ + try + { + if (process == null) + return true; + + bool completed = process.WaitForExit(timeoutMs); + if (!completed) + { + try { process.Kill(); } catch { } + } + return completed; + } + finally + { + process?.Dispose(); + } +}
🧹 Nitpick comments (3)
src/NLog.Targets.AtomicFile/AtomicFileTarget.cs (2)
94-122: Consider adding buffer size configuration for Unix implementation.The Windows implementations explicitly set buffer size to 1 (lines 71 and 82) to avoid internal buffering, but the Unix implementation doesn't specify a buffer size. Consider making this consistent across platforms.
- return new Mono.Unix.UnixStream(fd, true); + // Use no internal buffer, consistent with Windows implementation + return new Mono.Unix.UnixStream(fd, true) { BufferSize = 1 };
96-97: Make file permissions configurable instead of hardcoded.The current implementation uses hardcoded 0644 permissions. Consider making this configurable to support different security requirements.
+ /// <summary> + /// Gets or sets the file permissions for newly created log files (Unix only). + /// Default value is 0644 (owner read/write, group read, others read). + /// </summary> + public int UnixFilePermissions { get; set; } = 0644; private Stream CreateUnixStream(string filePath) { // Use permissions from configuration instead of hardcoded value - var permissions = (Mono.Unix.Native.FilePermissions)(6 | (6 << 3) | (6 << 6)); + var permissions = (Mono.Unix.Native.FilePermissions)UnixFilePermissions;tests/NLog.Targets.AtomicFile.Tests/ProcessRunner.cs (1)
113-113: Remove commented-out code.There's a commented-out line that uses a MemoryStream instead of a FileStream. Consider removing it if it's not needed, or add a comment explaining why it's kept.
using (var stream = new FileStream(outputAssembly, FileMode.CreateNew)) - //using (var stream = new MemoryStream()) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
appveyor.yml(0 hunks)build.ps1(1 hunks)run-tests.ps1(2 hunks)src/NLog.Targets.AtomicFile/AtomicFileTarget.cs(1 hunks)src/NLog.Targets.AtomicFile/NLog.Targets.AtomicFile.csproj(1 hunks)src/NLog.Targets.Network/Layouts/Log4JXmlEventLayout.cs(1 hunks)src/NLog.sln(3 hunks)src/NLog/Config/ConfigurationItemFactory.cs(1 hunks)src/NLog/Targets/ConsoleTargetHelper.cs(1 hunks)tests/NLog.Targets.AtomicFile.Tests/ConcurrentWritesMultiProcessTests.cs(1 hunks)tests/NLog.Targets.AtomicFile.Tests/NLog.Targets.AtomicFile.Tests.csproj(1 hunks)tests/NLog.Targets.AtomicFile.Tests/ProcessRunner.cs(1 hunks)tests/NLog.Targets.AtomicFile.Tests/Properties/AssemblyInfo.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentFileTargetTests.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentWritesMultiProcessTests.cs(3 hunks)tests/NLog.Targets.ConcurrentFile.Tests/FileAppenderCacheTests.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/FilePathLayoutTests.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/ProcessRunner.cs(1 hunks)
💤 Files with no reviewable changes (1)
- appveyor.yml
✅ Files skipped from review due to trivial changes (1)
- tests/NLog.Targets.AtomicFile.Tests/NLog.Targets.AtomicFile.Tests.csproj
🚧 Files skipped from review as they are similar to previous changes (13)
- tests/NLog.Targets.ConcurrentFile.Tests/ProcessRunner.cs
- tests/NLog.Targets.ConcurrentFile.Tests/FilePathLayoutTests.cs
- src/NLog/Targets/ConsoleTargetHelper.cs
- tests/NLog.Targets.ConcurrentFile.Tests/FileAppenderCacheTests.cs
- tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentFileTargetTests.cs
- tests/NLog.Targets.AtomicFile.Tests/Properties/AssemblyInfo.cs
- run-tests.ps1
- build.ps1
- src/NLog.Targets.AtomicFile/NLog.Targets.AtomicFile.csproj
- tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentWritesMultiProcessTests.cs
- src/NLog.sln
- src/NLog.Targets.Network/Layouts/Log4JXmlEventLayout.cs
- tests/NLog.Targets.AtomicFile.Tests/ConcurrentWritesMultiProcessTests.cs
🧰 Additional context used
🧬 Code Definitions (2)
tests/NLog.Targets.AtomicFile.Tests/ProcessRunner.cs (1)
tests/NLog.Targets.ConcurrentFile.Tests/ProcessRunner.cs (3)
ProcessRunner(46-135)ProcessRunner(48-101)Process(103-134)
src/NLog/Config/ConfigurationItemFactory.cs (2)
src/NLog/Config/Factory.cs (1)
CheckTypeAliasExists(85-85)src/NLog/Config/MethodFactory.cs (1)
CheckTypeAliasExists(106-106)
🔇 Additional comments (4)
src/NLog/Config/ConfigurationItemFactory.cs (2)
432-460: Added support for atomic file target.The changes correctly register the new AtomicFileTarget with two aliases: "atomfile" and "atomicfile". This follows the same pattern used for other targets and makes the new functionality discoverable.
463-469: Good refactoring with SafeRegisterNamedType.This helper method efficiently encapsulates the repeated pattern of checking if an alias exists before registering it, improving code maintainability by reducing duplication.
src/NLog.Targets.AtomicFile/AtomicFileTarget.cs (2)
41-48: Good implementation of target attributes and properties.The class is properly annotated with both "AtomFile" and "AtomicFile" target attributes, maintaining consistency with other NLog targets. The
ConcurrentWritesproperty is well-documented and has a sensible default value oftrue.
51-91: Well-structured platform-specific file stream creation.The implementation correctly handles different platforms with conditional compilation directives, ensuring atomic file append operations across different operating systems. The method elegantly falls back to the base implementation when
ConcurrentWritesis disabled or when not keeping the file open.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/NLog.Targets.AtomicFile.Tests/ConcurrentWritesMultiProcessTests.cs (1)
298-300:⚠️ Potential issueEmpty catch block swallows exceptions
The empty catch block in the cleanup section silently swallows exceptions. This could hide important error information during test failures.
Consider at least logging these exceptions or introducing a fallback mechanism:
catch { + // Log the cleanup failure but don't fail the test + System.Diagnostics.Debug.WriteLine("Failed to clean up test directories"); }
🧹 Nitpick comments (8)
src/NLog.Targets.Network/Layouts/GelfLayout.cs (1)
237-237: Consider renaming the local variable for clarity.
The variable namegelfSeverityis slightly misleading since the method returns aSyslogLevel. Consider renaming it togelfLevelfor consistency with the new naming.src/NLog.Targets.AtomicFile/AtomicFileTarget.cs (1)
52-84: Consider adding retry backoff
The current retry logic attempts five times without any delay between attempts, which may lead to repeated immediate failures under heavy concurrency. Consider introducing a small exponential backoff to improve reliability.for (int i = 1; i <= maxRetryCount; ++i) { try { return CreateAtomicFileStream(filePath); } catch (IOException ex) { + // Optionally add a delay before retry, e.g. exponential backoff + Thread.Sleep(50 * (int)Math.Pow(2, i - 1)); if (i == maxRetryCount) throw; } }tests/NLog.Targets.AtomicFile.Tests/ProcessRunner.cs (1)
131-162: Dispose the returned process
The method returns aProcessthat may remain un-disposed if the caller forgets. Consider providing a safe cleanup mechanism or an explicit usage pattern to avoid resource leaks.public static Process SpawnMethod(Type type, string methodName, params string[] p) { ... proc.Start(); + // Consider adding a using statement or a helper method to ensure disposal. return proc; }src/NLog/Config/ConfigurationItemFactory.cs (1)
444-445: Redundant aliases for atomic file target
Registering both "atomfile" and "atomicfile" might be intentional for backwards compatibility. If not required, consider removing one alias to reduce confusion.tests/NLog.Targets.AtomicFile.Tests/ConcurrentWritesMultiProcessTests.cs (4)
60-72: Remove unused mutex mode parsing logicThe
ConfigureSharedFilemethod parses for the "mutex" mode in line 63, but unlike the ConcurrentFileTarget implementation, it doesn't set any corresponding property on the AtomicFileTarget. This can cause confusion since the "mutex" mode appears to be recognized but doesn't affect the target's behavior.Since AtomicFileTarget doesn't appear to expose the ForceMutexConcurrentWrites property, remove the unnecessary "mutex" mode handling from your test method and update the test cases accordingly.
76-77: Archive file name format differs from ConcurrentFileTargetThe archive file naming scheme implemented here differs from the one in ConcurrentFileTarget, which may lead to inconsistent behavior between the two targets.
Consider aligning the archive file naming approach with ConcurrentFileTarget by using the same pattern (
{####}_filename) or document the intentional difference between the implementations.
245-246: Maximum file size check is more permissive than ConcurrentFileTargetThe file size verification threshold is set to 150 bytes here, while in the ConcurrentFileTarget tests it's set to 100 bytes.
Consider aligning the file size verification thresholds with ConcurrentFileTarget tests for consistency, or document why the threshold needs to be higher for AtomicFileTarget.
356-364: Static helper method never used directlyThe
ChildLoggerProcessmethod is defined but never directly called in the test class. It appears to be intended for use withProcessRunner.SpawnMethod(), but that method instead callsMultiProcessExecutordirectly.Either remove the unused
ChildLoggerProcessmethod or modifyProcessRunner.SpawnMethod()calls to use this method instead ofMultiProcessExecutor.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
build.ps1(1 hunks)run-tests.ps1(2 hunks)src/NLog.Targets.AtomicFile/AtomicFileTarget.cs(1 hunks)src/NLog.Targets.AtomicFile/NLog.Targets.AtomicFile.csproj(1 hunks)src/NLog.Targets.Network/Layouts/GelfLayout.cs(2 hunks)src/NLog.Targets.Network/Layouts/Log4JXmlEventLayout.cs(1 hunks)src/NLog.Targets.Network/Layouts/SyslogFacility.cs(2 hunks)src/NLog.Targets.Network/Layouts/SyslogLayout.cs(5 hunks)src/NLog.Targets.Network/Layouts/SyslogLevel.cs(1 hunks)src/NLog.Targets.Network/Targets/SyslogTarget.cs(1 hunks)src/NLog.sln(3 hunks)src/NLog/Config/ConfigurationItemFactory.cs(1 hunks)src/NLog/Targets/ColoredConsoleTarget.cs(1 hunks)src/NLog/Targets/ConsoleTargetHelper.cs(1 hunks)src/NLog/Targets/FileAppenders/ExclusiveFileLockingAppender.cs(1 hunks)tests/NLog.Targets.AtomicFile.Tests/ConcurrentWritesMultiProcessTests.cs(1 hunks)tests/NLog.Targets.AtomicFile.Tests/NLog.Targets.AtomicFile.Tests.csproj(1 hunks)tests/NLog.Targets.AtomicFile.Tests/ProcessRunner.cs(1 hunks)tests/NLog.Targets.AtomicFile.Tests/Properties/AssemblyInfo.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentFileTargetTests.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentWritesMultiProcessTests.cs(3 hunks)tests/NLog.Targets.ConcurrentFile.Tests/FileAppenderCacheTests.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/FilePathLayoutTests.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/ProcessRunner.cs(1 hunks)tests/NLog.Targets.Network.Tests/GelfLayoutTests.cs(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (19)
- src/NLog/Targets/ConsoleTargetHelper.cs
- tests/NLog.Targets.Network.Tests/GelfLayoutTests.cs
- tests/NLog.Targets.AtomicFile.Tests/Properties/AssemblyInfo.cs
- tests/NLog.Targets.ConcurrentFile.Tests/FileAppenderCacheTests.cs
- src/NLog.Targets.Network/Targets/SyslogTarget.cs
- src/NLog/Targets/FileAppenders/ExclusiveFileLockingAppender.cs
- run-tests.ps1
- src/NLog.Targets.Network/Layouts/Log4JXmlEventLayout.cs
- tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentFileTargetTests.cs
- tests/NLog.Targets.ConcurrentFile.Tests/FilePathLayoutTests.cs
- build.ps1
- src/NLog.sln
- src/NLog/Targets/ColoredConsoleTarget.cs
- tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentWritesMultiProcessTests.cs
- src/NLog.Targets.Network/Layouts/SyslogLevel.cs
- src/NLog.Targets.AtomicFile/NLog.Targets.AtomicFile.csproj
- tests/NLog.Targets.AtomicFile.Tests/NLog.Targets.AtomicFile.Tests.csproj
- tests/NLog.Targets.ConcurrentFile.Tests/ProcessRunner.cs
- src/NLog.Targets.Network/Layouts/SyslogFacility.cs
🧰 Additional context used
🧬 Code Definitions (3)
tests/NLog.Targets.AtomicFile.Tests/ProcessRunner.cs (1)
tests/NLog.Targets.ConcurrentFile.Tests/ProcessRunner.cs (3)
ProcessRunner(46-135)ProcessRunner(48-101)Process(103-134)
tests/NLog.Targets.AtomicFile.Tests/ConcurrentWritesMultiProcessTests.cs (2)
tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentWritesMultiProcessTests.cs (10)
ConcurrentWritesMultiProcessTests(53-372)ConcurrentWritesMultiProcessTests(57-62)ConfigureSharedFile(64-110)MultiProcessExecutor(113-170)MakeFileName(172-176)DoConcurrentTest(178-306)Theory(308-335)Theory(337-351)Theory(353-361)Theory(363-371)tests/NLog.Targets.ConcurrentFile.Tests/ProcessRunner.cs (3)
Process(103-134)ProcessRunner(46-135)ProcessRunner(48-101)
src/NLog.Targets.Network/Layouts/GelfLayout.cs (1)
src/NLog.Targets.Network/Layouts/SyslogLayout.cs (1)
SyslogLevel(488-498)
🔇 Additional comments (19)
src/NLog.Targets.Network/Layouts/GelfLayout.cs (2)
456-466: Fallback exception handling looks good.
The try/catch block nicely safeguards against out-of-range Ordinal values, mapping them toSyslogLevel.Emergency. This is consistent with the approach inSyslogLayout.
468-477: Confirm the correctness of the log level mapping array.
Please verify that mapping bothLogLevel.FatalandLogLevel.OfftoSyslogLevel.Emergencyis the intended behavior, and that the duplicated entries forTraceandDebugboth mapping toSyslogLevel.Debugare aligned with your requirements.src/NLog.Targets.Network/Layouts/SyslogLayout.cs (10)
121-121: Implementation of SyslogLevel property is valid.
DefiningSyslogLevelas a thread-agnostic layout property is a concise way to map NLog’s LogLevel to SyslogLevel dynamically.
144-144: Potential concurrency issue when reassigning_priValueMapping.
_priValueMappingis overwritten whenever a new facility is encountered, which can cause race conditions in multi-threaded scenarios ifRenderFormattedMessageis invoked concurrently. Consider synchronizing access to this field or storing mappings in a thread-safe manner.
181-182: Adding SyslogLevel to the layout collection looks good.
Including the newSyslogLevellayout inLayoutsensures it is initialized and properly rendered.
209-209: Retrieving syslog level is straightforward.
CallingSyslogLevel.RenderValue(logEvent)is an appropriate way to get the computed level for the current log event.
215-215: Mapping facility context is properly recalculated.
A new facility mapper is resolved if the cached key does not match, ensuring the correctpriValuefor the new facility. If multi-threading is expected, also consider thread-safe updates here (same concurrency note as above).
219-219: Dictionary index usage.
AccessingpriValueMapper.Value[syslogLevel]is safe given the completeness of theResolveFacilityMapperdictionary for all syslog levels.
477-486: Confirm the loglevel array coverage.
This array aligns NLogLogLevelordinals toSyslogLevel. Double-check that the chosen mapping forFatalandOfftoEmergencyaligns with your design intentions.
488-499: Good fallback to Emergency level.
Just like inGelfLayout, the catch block for an invalidOrdinalproperly defaults toSyslogLevel.Emergency.
500-513: Well-structured approach for facility-based dictionary lookup.
Generating a new dictionary per facility helps maintain clarity and ensures each SyslogLevel resolves properly to a<PRI>value.
515-519: Simple arithmetic for<PRI>header is correct.
<facility * 8 + severity>aligns with the standard syslog protocol for priority calculation, producing correct facility and level bits.src/NLog.Targets.AtomicFile/AtomicFileTarget.cs (3)
45-50: Clarify concurrency usage in property doc
The property nameConcurrentWritesimplies concurrency is fully handled. It might be beneficial to mention concurrency caveats in the docstring, e.g., potential platform limitations or scenarios where concurrency might still cause file locking issues.Please verify concurrency on different platforms (Windows, Linux, etc.) to confirm consistent behavior.
86-122: LGTM
The logic for creating atomic file streams under different frameworks is clean and straightforward.
125-130: Check file permission scope
Using0666grants global read/write, which might be more permissive than necessary. Confirm this aligns with your security requirements.tests/NLog.Targets.AtomicFile.Tests/ProcessRunner.cs (1)
50-90: No issues found in reflection-based main
The dynamically compiled code forC1.Main()is consistent with typical reflection-based test harness patterns.src/NLog/Config/ConfigurationItemFactory.cs (2)
429-437: Centralize named-type registration
This helper method nicely reduces duplicated logic and ensures consistent behavior when registering named types.
442-443: LGTM for target and layout registrations
All named-type registrations appear correct and consistent.Also applies to: 446-468, 474-479, 486-487, 489-496
tests/NLog.Targets.AtomicFile.Tests/ConcurrentWritesMultiProcessTests.cs (1)
304-311: Missing mutex tests compared to ConcurrentFileTargetThe ConcurrentFileTarget tests include several mutex-related test cases (e.g.,
[InlineData(2, 10000, "none|mutex")]) that are missing here. Without these tests, the behavior of AtomicFileTarget in mutex-related scenarios remains unverified.If the AtomicFileTarget is designed to work without mutex synchronization, please:
- Document this design decision in comments
- Remove any unused mutex-related parsing logic in the ConfigureSharedFile method
- Explain how AtomicFileTarget ensures atomicity without mutex synchronization
If mutex synchronization should be supported, add the appropriate test cases similar to ConcurrentFileTarget.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
src/NLog.Targets.AtomicFile/AtomicFileTarget.cs (1)
125-153:⚠️ Potential issueFile permissions inconsistency in Unix implementation.
The comment indicates permissions of 0644 (read/write for owner, read for group and others), but the actual implementation sets 0666 (read/write for all), which is more permissive than needed.
- // Use 0666 (read/write for all) - var permissions = (Mono.Unix.Native.FilePermissions)(6 | (6 << 3) | (6 << 6)); + // Use 0644 permissions (read/write for owner, read for group and others) + var permissions = (Mono.Unix.Native.FilePermissions)(4 | (4 << 3) | (6 << 6));tests/NLog.Targets.AtomicFile.Tests/ProcessRunner.cs (4)
100-100:⚠️ Potential issueAdd a null check for the TEMP environment variable.
The current implementation could throw an exception if the TEMP environment variable is not set.
-options.TempFiles = new TempFileCollection(Environment.GetEnvironmentVariable("TEMP"), true); +var tempPath = Environment.GetEnvironmentVariable("TEMP") ?? Path.GetTempPath(); +options.TempFiles = new TempFileCollection(tempPath, true);
110-112:⚠️ Potential issueAdd error handling for file operations.
The file deletion and creation operations could throw exceptions if the file is in use or cannot be accessed. It would be better to wrap these in try-catch blocks and use a safer file mode.
-if (System.IO.File.Exists(outputAssembly)) - System.IO.File.Delete(outputAssembly); -using (var stream = new FileStream(outputAssembly, FileMode.CreateNew)) +try +{ + if (System.IO.File.Exists(outputAssembly)) + System.IO.File.Delete(outputAssembly); +} +catch (IOException ex) +{ + // Log the exception but continue - we'll try to overwrite the file + Console.WriteLine($"Warning: Could not delete existing Runner.dll: {ex.Message}"); +} +using (var stream = new FileStream(outputAssembly, FileMode.Create))
120-127:⚠️ Potential issueAdd error handling for missing runtimeconfig.json.
The code should check if the source runtimeconfig.json file exists before attempting to copy it.
var outputRuntimeConfig = Path.Combine(Path.GetDirectoryName(typeof(ProcessRunner).Assembly.Location), "Runner.runtimeconfig.json"); if (System.IO.File.Exists(outputRuntimeConfig)) System.IO.File.Delete(outputRuntimeConfig); -using (var streamReader = new StreamReader(Path.Combine(Path.GetDirectoryName(typeof(ProcessRunner).Assembly.Location), typeof(ProcessRunner).Assembly.GetName().Name) + ".runtimeconfig.json")) -using (var streamWriter = new StreamWriter(outputRuntimeConfig)) -{ - streamWriter.Write(streamReader.ReadToEnd()); -} +var sourceRuntimeConfig = Path.Combine(Path.GetDirectoryName(typeof(ProcessRunner).Assembly.Location), typeof(ProcessRunner).Assembly.GetName().Name + ".runtimeconfig.json"); +if (System.IO.File.Exists(sourceRuntimeConfig)) +{ + using (var streamReader = new StreamReader(sourceRuntimeConfig)) + using (var streamWriter = new StreamWriter(outputRuntimeConfig)) + { + streamWriter.Write(streamReader.ReadToEnd()); + } +} +else +{ + // Create a minimal runtimeconfig.json if source doesn't exist + using (var streamWriter = new StreamWriter(outputRuntimeConfig)) + { + streamWriter.Write("{ \"runtimeOptions\": { \"tfm\": \"netcoreapp3.1\", \"framework\": { \"name\": \"Microsoft.NETCore.App\", \"version\": \"3.1.0\" } } }"); + } +}
131-162: 🛠️ Refactor suggestionAdd process resource cleanup.
The
SpawnMethodreturns a Process object but doesn't ensure it gets properly cleaned up. Consider adding a utility method to safely dispose of the process or documentation to remind callers to handle cleanup.public static Process SpawnMethod(Type type, string methodName, params string[] p) { // existing code... proc.Start(); return proc; } +/// <summary> +/// Safely waits for a process to complete and releases resources +/// </summary> +/// <param name="process">The process to wait for</param> +/// <param name="timeoutMs">Timeout in milliseconds, defaults to 30 seconds</param> +/// <returns>True if process completed, false if it timed out</returns> +public static bool WaitAndCleanup(Process process, int timeoutMs = 30000) +{ + try + { + if (process == null) + return true; + + bool completed = process.WaitForExit(timeoutMs); + if (!completed) + { + try { process.Kill(); } catch { } + } + return completed; + } + finally + { + process?.Dispose(); + } +}
🧹 Nitpick comments (2)
src/NLog.Targets.AtomicFile/AtomicFileTarget.cs (2)
52-84: Consider adding a configurable retry count parameter for file stream creation.The retry logic for file stream creation uses a hard-coded constant of 5 attempts. Consider making this configurable to allow users to adjust based on their specific environment characteristics.
- const int maxRetryCount = 5; + /// <summary> + /// Gets or sets the maximum number of retries when attempting to create the file stream. + /// </summary> + public int MaxRetryCount { get; set; } = 5; + + // Then in method: + for (int i = 1; i <= MaxRetryCount; ++i)
86-123: Consider explaining the buffer size choice in the code comments.The implementation uses a buffer size of 1 for both Windows and .NET Framework implementations with the comment "No internal buffer, write directly from user-buffer". Consider adding more explanation about the performance implications of this choice for better maintainability.
- bufferSize: 1, // No internal buffer, write directly from user-buffer + bufferSize: 1, // No internal buffer, write directly from user-buffer. This avoids double-buffering and ensures atomic writes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
build.ps1(1 hunks)run-tests.ps1(2 hunks)src/NLog.Targets.AtomicFile/AtomicFileTarget.cs(1 hunks)src/NLog.Targets.AtomicFile/NLog.Targets.AtomicFile.csproj(1 hunks)src/NLog.Targets.Network/Layouts/GelfLayout.cs(2 hunks)src/NLog.Targets.Network/Layouts/Log4JXmlEventLayout.cs(1 hunks)src/NLog.Targets.Network/Layouts/SyslogFacility.cs(2 hunks)src/NLog.Targets.Network/Layouts/SyslogLayout.cs(5 hunks)src/NLog.Targets.Network/Layouts/SyslogLevel.cs(1 hunks)src/NLog.Targets.Network/Targets/SyslogTarget.cs(1 hunks)src/NLog.sln(3 hunks)src/NLog/Config/ConfigurationItemFactory.cs(1 hunks)src/NLog/LayoutRenderers/EventPropertiesLayoutRenderer.cs(1 hunks)src/NLog/LayoutRenderers/LoggerNameLayoutRenderer.cs(1 hunks)src/NLog/LayoutRenderers/Wrappers/ReplaceNewLinesLayoutRendererWrapper.cs(1 hunks)src/NLog/Targets/ColoredConsoleTarget.cs(1 hunks)src/NLog/Targets/ConsoleTargetHelper.cs(1 hunks)src/NLog/Targets/FileAppenders/ExclusiveFileLockingAppender.cs(1 hunks)tests/NLog.Targets.AtomicFile.Tests/ConcurrentWritesMultiProcessTests.cs(1 hunks)tests/NLog.Targets.AtomicFile.Tests/NLog.Targets.AtomicFile.Tests.csproj(1 hunks)tests/NLog.Targets.AtomicFile.Tests/ProcessRunner.cs(1 hunks)tests/NLog.Targets.AtomicFile.Tests/Properties/AssemblyInfo.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentFileTargetTests.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentWritesMultiProcessTests.cs(3 hunks)tests/NLog.Targets.ConcurrentFile.Tests/FileAppenderCacheTests.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/FilePathLayoutTests.cs(1 hunks)tests/NLog.Targets.ConcurrentFile.Tests/ProcessRunner.cs(1 hunks)tests/NLog.Targets.Network.Tests/GelfLayoutTests.cs(5 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/NLog/LayoutRenderers/Wrappers/ReplaceNewLinesLayoutRendererWrapper.cs
- src/NLog/LayoutRenderers/LoggerNameLayoutRenderer.cs
- src/NLog/LayoutRenderers/EventPropertiesLayoutRenderer.cs
🚧 Files skipped from review as they are similar to previous changes (22)
- tests/NLog.Targets.ConcurrentFile.Tests/FilePathLayoutTests.cs
- src/NLog.Targets.Network/Layouts/Log4JXmlEventLayout.cs
- tests/NLog.Targets.AtomicFile.Tests/Properties/AssemblyInfo.cs
- src/NLog/Targets/FileAppenders/ExclusiveFileLockingAppender.cs
- tests/NLog.Targets.ConcurrentFile.Tests/FileAppenderCacheTests.cs
- src/NLog/Targets/ConsoleTargetHelper.cs
- src/NLog.Targets.Network/Targets/SyslogTarget.cs
- tests/NLog.Targets.ConcurrentFile.Tests/ProcessRunner.cs
- tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentFileTargetTests.cs
- run-tests.ps1
- build.ps1
- tests/NLog.Targets.Network.Tests/GelfLayoutTests.cs
- src/NLog/Targets/ColoredConsoleTarget.cs
- src/NLog.Targets.AtomicFile/NLog.Targets.AtomicFile.csproj
- src/NLog.sln
- tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentWritesMultiProcessTests.cs
- src/NLog.Targets.Network/Layouts/SyslogLevel.cs
- src/NLog.Targets.Network/Layouts/GelfLayout.cs
- tests/NLog.Targets.AtomicFile.Tests/NLog.Targets.AtomicFile.Tests.csproj
- src/NLog.Targets.Network/Layouts/SyslogFacility.cs
- tests/NLog.Targets.AtomicFile.Tests/ConcurrentWritesMultiProcessTests.cs
- src/NLog.Targets.Network/Layouts/SyslogLayout.cs
🧰 Additional context used
🧬 Code Definitions (2)
src/NLog/Config/ConfigurationItemFactory.cs (3)
src/NLog/Config/Factory.cs (4)
TBaseType(225-276)Factory(48-221)Factory(58-62)CheckTypeAliasExists(85-85)src/NLog/Config/MethodFactory.cs (1)
CheckTypeAliasExists(106-106)src/NLog/Config/AssemblyExtensionTypes.cs (4)
AssemblyExtensionTypes(39-398)RegisterTargetTypes(41-97)RegisterLayoutTypes(99-120)RegisterLayoutRendererTypes(122-341)
tests/NLog.Targets.AtomicFile.Tests/ProcessRunner.cs (1)
tests/NLog.Targets.ConcurrentFile.Tests/ProcessRunner.cs (3)
ProcessRunner(46-135)ProcessRunner(48-101)Process(103-134)
🔇 Additional comments (3)
src/NLog.Targets.AtomicFile/AtomicFileTarget.cs (1)
40-50: Well-designed target class with appropriate attributes and documentation.The implementation of a new AtomicFileTarget with
[Target]attributes for both "AtomFile" and "AtomicFile" offers good flexibility. The documentation clearly explains the purpose of the target for atomic file append operations in multi-process scenarios.src/NLog/Config/ConfigurationItemFactory.cs (2)
429-437: Good refactoring with the SafeRegisterNamedType method.The introduction of the
SafeRegisterNamedTypemethod reduces code duplication and improves maintainability by encapsulating the type registration logic in a single method.
442-467:Details
✅ Verification successful
Verify the atomfile and atomicfile target registrations.
Ensure that the target names "atomfile" and "atomicfile" are correctly registered to point to the AtomicFileTarget type. This is critical for proper configuration loading.
🏁 Script executed:
#!/bin/bash # Check if AtomicFileTarget is correctly registered and referenced # Look for the AtomicFileTarget class to confirm it exists echo "Checking for AtomicFileTarget class definition:" rg -A 3 "class AtomicFileTarget" --type cs # Check for Target attribute definitions in AtomicFileTarget echo "Checking Target attributes in AtomicFileTarget:" rg "\[Target\(\"Atom" --type cs # Check for usage patterns or tests that verify the registration works echo "Checking for test/usage of atomfile/atomicfile targets:" rg "atomfile|atomicfile" --type csLength of output: 1340
AtomicFileTarget Registrations Verified
The registrations for both
atomfileandatomicfilehave been confirmed to correctly referenceNLog.Targets.AtomicFileTarget, NLog.Targets.AtomicFile. TheAtomicFileTargetclass exists as expected, with the appropriate[Target("AtomFile")]and[Target("AtomicFile")]attributes, and the usage within the configuration file aligns with these registrations. No changes are required here.
05f9bf9 to
d48cb02
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/NLog.Targets.AtomicFile.Tests/ConcurrentWritesMultiProcessTests.cs (2)
49-52: Consider sharing concurrency test logic with ConcurrentFileTarget.These tests appear structurally similar to those in
tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentWritesMultiProcessTests.cs. To avoid duplication and improve maintainability, consider factoring out common concurrency test logic into shared utilities or base classes, so both targets can leverage a unified test approach.
243-252: Avoid using arbitrary file size checks that might cause flaky tests.Enforcing size limits (e.g., 35–150 bytes) can break under different environments or timing variances in concurrency scenarios. A more robust validation approach (e.g., verifying exact line counts) will reduce the risk of intermittent test failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
run-tests.ps1(2 hunks)tests/NLog.Targets.AtomicFile.Tests/ConcurrentWritesMultiProcessTests.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- run-tests.ps1
🧰 Additional context used
🧬 Code Definitions (1)
tests/NLog.Targets.AtomicFile.Tests/ConcurrentWritesMultiProcessTests.cs (1)
tests/NLog.Targets.ConcurrentFile.Tests/ConcurrentWritesMultiProcessTests.cs (10)
ConcurrentWritesMultiProcessTests(53-372)ConcurrentWritesMultiProcessTests(57-62)ConfigureSharedFile(64-110)MultiProcessExecutor(113-170)MakeFileName(172-176)DoConcurrentTest(178-306)Theory(308-335)Theory(337-351)Theory(353-361)Theory(363-371)
🔇 Additional comments (2)
tests/NLog.Targets.AtomicFile.Tests/ConcurrentWritesMultiProcessTests.cs (2)
316-318: Empty catch block duplicates previously flagged issue.This empty catch block in the cleanup phase still silently discards exceptions, which may hide important diagnostic information. Consider at least logging the exception before swallowing it.
322-345: Missing “mutex” test scenario duplicates previously flagged issue.Unlike the ConcurrentFileTarget tests, which include scenarios with a “mutex” parameter, this suite skips mutex-based concurrent writes. If you plan to support similar locking modes for AtomicFileTarget, consider adding test coverage for these scenarios to ensure consistent behavior.
14fff65 to
a52a813
Compare
|
|
Created wiki: https://github.com/NLog/NLog/wiki/Atomic-File-Target |
Trying to resolve #4244 by supporting O_APPEND for Linux with help from
Mono.Posix.NETStandardLinux must use
dotnet publishwith--framework net8.0 --configuration release --runtime linux-x64Notice that there is no global Mutex to protect, when using legacy archive-logic with
archiveFileName="...", so archive-logic with file-move can trigger file-lock issues when multiple processes. But the legacy FileTarget was not fool proof either, as unit-test with concurrent archive-logic behaved unstable.