diff --git a/docs/code-review/02-joblogger-thread-safety.md b/docs/code-review/02-joblogger-thread-safety.md index 1366cc5..299fd22 100644 --- a/docs/code-review/02-joblogger-thread-safety.md +++ b/docs/code-review/02-joblogger-thread-safety.md @@ -2,7 +2,7 @@ **Priority:** 🔴 Critical **File:** `src/ArmRipper.Core/Infrastructure/JobLogger.cs` -**Status:** ⬜ Todo +**Status:** ✅ Done --- diff --git a/docs/code-review/README.md b/docs/code-review/README.md index fc94896..5377cdd 100644 --- a/docs/code-review/README.md +++ b/docs/code-review/README.md @@ -25,7 +25,7 @@ proposed fix. | # | Task | Priority | Status | Assignee | |---|------|----------|--------|----------| | 1 | [Fix sync-over-async in `StartImportJob`](01-fix-sync-over-async.md) | 🔴 Critical | ✅ Done | — | -| 2 | [Fix `JobLogger` thread-safety](02-joblogger-thread-safety.md) | 🔴 Critical | ⬜ Todo | — | +| 2 | [Fix `JobLogger` thread-safety](02-joblogger-thread-safety.md) | 🔴 Critical | ✅ Done | — | | 3 | [Fix `CheckMediaPresent` async gap & missing failure state](03-checkmedia-async.md) | 🔴 Critical | ⬜ Todo | — | ### 🟡 Medium (correctness / maintainability) @@ -84,9 +84,9 @@ proposed fix. | Status | Count | |--------|-------| -| ⬜ Todo | 26 | +| ⬜ Todo | 25 | | 🔄 In Progress | 0 | -| ✅ Done | 10 | +| ✅ Done | 11 | --- diff --git a/src/ArmRipper.Core/Infrastructure/JobLogger.cs b/src/ArmRipper.Core/Infrastructure/JobLogger.cs index 2e45abe..c5041c1 100644 --- a/src/ArmRipper.Core/Infrastructure/JobLogger.cs +++ b/src/ArmRipper.Core/Infrastructure/JobLogger.cs @@ -8,6 +8,8 @@ public sealed class JobLogger : ILogger private readonly string _logPath; private readonly StreamWriter _fileWriter; private readonly ILogger _inner; + private readonly object _writeLock = new(); + private bool _disposed; public JobLogger(string jobId, string logDirectory, ILogger inner) { @@ -21,7 +23,11 @@ public JobLogger(string jobId, string logDirectory, ILogger inner) public void Log(LogLevel logLevel, EventId eventId, TState state, Exception? exception, Func formatter) { var line = $"[{DateTime.Now:yyyy-MM-dd HH:mm:ss}] [{logLevel}] {formatter(state, exception)}"; - _fileWriter.WriteLine(line); + lock (_writeLock) + { + if (_disposed) return; + _fileWriter.WriteLine(line); + } _inner.Log(logLevel, eventId, state, exception, formatter); } @@ -31,13 +37,18 @@ public void Log(LogLevel logLevel, EventId eventId, TState state, Except public void Dispose() { - _fileWriter.Flush(); - _fileWriter.Dispose(); + lock (_writeLock) + { + if (_disposed) return; + _disposed = true; + _fileWriter.Flush(); + _fileWriter.Dispose(); + } } - public async ValueTask DisposeAsync() + public ValueTask DisposeAsync() { - await _fileWriter.FlushAsync(); - await _fileWriter.DisposeAsync(); + Dispose(); + return ValueTask.CompletedTask; } } diff --git a/tests/ArmRipper.Core.Tests/JobLoggerTests.cs b/tests/ArmRipper.Core.Tests/JobLoggerTests.cs new file mode 100644 index 0000000..7150e21 --- /dev/null +++ b/tests/ArmRipper.Core.Tests/JobLoggerTests.cs @@ -0,0 +1,50 @@ +using ArmRipper.Core.Infrastructure; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; + +namespace ArmRipper.Core.Tests; + +public sealed class JobLoggerTests +{ + private static JobLogger CreateLogger(out string logPath) + { + var dir = Path.Combine(Path.GetTempPath(), $"joblogger-{Guid.NewGuid():N}"); + var jobId = Guid.NewGuid().ToString("N"); + logPath = Path.Combine(dir, $"arm_job_{jobId}.log"); + return new JobLogger(jobId, dir, NullLogger.Instance); + } + + [Fact] + public async Task ConcurrentLogs_AllLinesPreserved() + { + var logger = CreateLogger(out var logPath); + + const int messageCount = 200; + var messages = Enumerable.Range(0, messageCount) + .Select(i => $"message-{i}-{new string('x', 100)}") + .ToArray(); + + await Task.WhenAll(messages.Select(m => + Task.Run(() => logger.Log(LogLevel.Information, 0, m, null, (s, _) => s!)))); + + logger.Dispose(); + + var lines = await File.ReadAllLinesAsync(logPath); + var recovered = lines.Select(l => l.Split("] ").Last()).ToArray(); + + Assert.Equal(messageCount, lines.Length); + Assert.Equal(messages.OrderBy(m => m), recovered.OrderBy(m => m)); + } + + [Fact] + public void Log_AfterDispose_DoesNotThrow() + { + var logger = CreateLogger(out var logPath); + logger.Dispose(); + + var ex = Record.Exception(() => + logger.Log(LogLevel.Information, 0, "late message", null, (s, _) => s!)); + + Assert.Null(ex); + } +}