Skip to content

Commit c01453f

Browse files
author
BRUNER Patrick
committed
update review
1 parent c60ac30 commit c01453f

6 files changed

Lines changed: 135 additions & 78 deletions

File tree

‎src/LogExpert.Core/Classes/Log/Buffers/LogBuffer.cs‎

Lines changed: 29 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -90,8 +90,8 @@ public long Size
9090
#region Public methods
9191

9292
/// <summary>
93-
/// Increments the pin count. While pinned, the buffer will not be evicted by the LRU garbage collector.
94-
/// Each call to Pin() must be balanced by a call to Unpin().
93+
/// Increments the pin count. While pinned, the buffer will not be evicted by the LRU garbage collector. Each call
94+
/// to Pin() must be balanced by a call to Unpin().
9595
/// </summary>
9696
public void Pin ()
9797
{
@@ -103,20 +103,24 @@ public void Pin ()
103103
/// </summary>
104104
public void Unpin ()
105105
{
106-
var newCount = Interlocked.Decrement(ref _pinCount);
107106
#if DEBUG
107+
var newCount = Interlocked.Decrement(ref _pinCount);
108108
if (newCount < 0)
109109
{
110110
_logger.Warn("Unpin underflow: _pinCount went to {0}. Unbalanced Pin/Unpin calls.", newCount);
111111
}
112+
#else
113+
Interlocked.Decrement(ref _pinCount);
112114
#endif
113115
}
114116

115117
/// <summary>
116118
/// Adds a log line to the internal collection at the specified file position.
117119
/// </summary>
118-
/// <remarks>If the internal collection has reached its maximum capacity, the log line is not added. In
119-
/// debug builds, an error is logged when this occurs.</remarks>
120+
/// <remarks>
121+
/// If the internal collection has reached its maximum capacity, the log line is not added. In debug builds, an
122+
/// error is logged when this occurs.
123+
/// </remarks>
120124
/// <param name="lineMemory">The log line to add to the collection.</param>
121125
/// <param name="filePos">The file position associated with the log line.</param>
122126
public void AddLine (LogLine lineMemory, long filePos)
@@ -142,9 +146,11 @@ public void AddLine (LogLine lineMemory, long filePos)
142146
/// <summary>
143147
/// Removes all log lines from the current collection, resetting its state for reuse.
144148
/// </summary>
145-
/// <remarks>After calling this method, the collection will be empty and ready to accept new log lines.
146-
/// Any resources associated with the previous log lines are released. This method is typically used to clear the
147-
/// log data before loading new content or starting a new logging session.</remarks>
149+
/// <remarks>
150+
/// After calling this method, the collection will be empty and ready to accept new log lines. Any resources
151+
/// associated with the previous log lines are released. This method is typically used to clear the log data before
152+
/// loading new content or starting a new logging session.
153+
/// </remarks>
148154
public void ClearLines ()
149155
{
150156
if (_lineArray == null)
@@ -238,9 +244,13 @@ public void DisposeContent ()
238244
/// <summary>
239245
/// Retrieves the log line at the specified index within the current memory block.
240246
/// </summary>
241-
/// <param name="num">The zero-based index of the log line to retrieve. Must be greater than or equal to 0 and less than the total
242-
/// number of lines.</param>
243-
/// <returns>The <see cref="LogLine"/> at the specified index if it exists; otherwise, <see langword="null"/>.</returns>
247+
/// <param name="num">
248+
/// The zero-based index of the log line to retrieve. Must be greater than or equal to 0 and less than the total
249+
/// number of lines.
250+
/// </param>
251+
/// <returns>
252+
/// The <see cref="LogLine"/> at the specified index if it exists; otherwise, <see langword="null"/>.
253+
/// </returns>
244254
public LogLine? GetLineMemoryOfBlock (int num)
245255
{
246256
return num < LineCount && num >= 0
@@ -266,10 +276,9 @@ public void ReleaseContentLock ()
266276

267277
/// <summary>
268278
/// Attaches pooled char[] blocks that back the ReadOnlyMemory in this buffer's LogLine entries. These blocks will
269-
/// be returned to ArrayPool when the buffer is evicted or disposed.
270-
/// New blocks are MERGED with existing ones — never replace — because the buffer's existing
271-
/// LogLine entries still reference the old blocks (e.g., during tail mode where multiple
272-
/// read sessions append lines to the same buffer).
279+
/// be returned to ArrayPool when the buffer is evicted or disposed. New blocks are MERGED with existing ones —
280+
/// never replace — because the buffer's existing LogLine entries still reference the old blocks (e.g., during tail
281+
/// mode where multiple read sessions append lines to the same buffer).
273282
/// </summary>
274283
public void AttachCharBlocks (List<char[]> blocks)
275284
{
@@ -304,9 +313,11 @@ public long GetFilePosForLineOfBlock (int line)
304313
/// Releases references to the character block buffers used by this instance, allowing them to be garbage collected
305314
/// when no longer in use.
306315
/// </summary>
307-
/// <remarks>If the buffer is pinned, this method only drops the reference without returning the blocks to
308-
/// the array pool, as external consumers may still hold references. This helps prevent premature reuse of buffers
309-
/// that may still be accessed elsewhere.</remarks>
316+
/// <remarks>
317+
/// If the buffer is pinned, this method only drops the reference without returning the blocks to the array pool, as
318+
/// external consumers may still hold references. This helps prevent premature reuse of buffers that may still be
319+
/// accessed elsewhere.
320+
/// </remarks>
310321
private void ReturnCharBlocks ()
311322
{
312323
if (_charBlocks is null)

‎src/LogExpert.Core/Classes/Log/LogfileReader.cs‎

Lines changed: 49 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -1376,87 +1376,80 @@ private void ReReadBuffer (LogBuffer logBuffer)
13761376
#endif
13771377
lock (_logBufferLock)
13781378
{
1379-
Stream fileStream = null;
1379+
Stream openendFileStream;
13801380
try
13811381
{
1382-
fileStream = logBuffer.FileInfo.OpenStream();
1382+
openendFileStream = logBuffer.FileInfo.OpenStream();
13831383
}
13841384
catch (IOException e)
13851385
{
13861386
_logger.Warn(e);
13871387
return;
13881388
}
13891389

1390-
ILogStreamReaderMemory reader = null;
1391-
try
1390+
using Stream fileStream = openendFileStream;
13921391
{
13931392
//TODO LogStream Reader has to be changed to ILogStreamReaderMemory
1394-
reader = GetLogStreamReader(fileStream, EncodingOptions) as ILogStreamReaderMemory;
1393+
var reader = GetLogStreamReader(fileStream, EncodingOptions) as ILogStreamReaderMemory;
13951394

1396-
var filePos = logBuffer.StartPos;
1397-
reader.Position = logBuffer.StartPos;
1398-
var maxLinesCount = logBuffer.LineCount;
1399-
var lineCount = 0;
1400-
var dropCount = logBuffer.PrevBuffersDroppedLinesSum;
1401-
logBuffer.ClearLines();
1395+
using var readerDisposabel = reader as IDisposable;
1396+
{
1397+
var filePos = logBuffer.StartPos;
1398+
reader.Position = logBuffer.StartPos;
1399+
var maxLinesCount = logBuffer.LineCount;
1400+
var lineCount = 0;
1401+
var dropCount = logBuffer.PrevBuffersDroppedLinesSum;
1402+
logBuffer.ClearLines();
14021403

1403-
var (success, lineMemory, wasDropped) = ReadLineMemory(reader, logBuffer.StartLine + logBuffer.LineCount, logBuffer.StartLine + logBuffer.LineCount + dropCount);
1404+
var (success, lineMemory, wasDropped) = ReadLineMemory(reader, logBuffer.StartLine + logBuffer.LineCount, logBuffer.StartLine + logBuffer.LineCount + dropCount);
14041405

1405-
while (success)
1406-
{
1407-
if (lineCount >= maxLinesCount)
1406+
while (success)
14081407
{
1409-
break;
1410-
}
1408+
if (lineCount >= maxLinesCount)
1409+
{
1410+
break;
1411+
}
14111412

1412-
if (wasDropped)
1413-
{
1414-
dropCount++;
1415-
(success, lineMemory, wasDropped) = ReadLineMemory(reader, logBuffer.StartLine + logBuffer.LineCount, logBuffer.StartLine + logBuffer.LineCount + dropCount);
1416-
continue;
1417-
}
1413+
if (wasDropped)
1414+
{
1415+
dropCount++;
1416+
(success, lineMemory, wasDropped) = ReadLineMemory(reader, logBuffer.StartLine + logBuffer.LineCount, logBuffer.StartLine + logBuffer.LineCount + dropCount);
1417+
continue;
1418+
}
14181419

1419-
LogLine logLine = new(lineMemory, logBuffer.StartLine + logBuffer.LineCount);
1420+
LogLine logLine = new(lineMemory, logBuffer.StartLine + logBuffer.LineCount);
14201421

1421-
logBuffer.AddLine(logLine, filePos);
1422-
filePos = reader.Position;
1423-
lineCount++;
1422+
logBuffer.AddLine(logLine, filePos);
1423+
filePos = reader.Position;
1424+
lineCount++;
14241425

1425-
(success, lineMemory, wasDropped) = ReadLineMemory(reader, logBuffer.StartLine + logBuffer.LineCount, logBuffer.StartLine + logBuffer.LineCount + dropCount);
1426-
}
1426+
(success, lineMemory, wasDropped) = ReadLineMemory(reader, logBuffer.StartLine + logBuffer.LineCount, logBuffer.StartLine + logBuffer.LineCount + dropCount);
1427+
}
14271428

1428-
// Attach char blocks from the reader to the re-read buffer
1429-
if (reader is PositionAwareStreamReaderSystem systemReader)
1430-
{
1431-
logBuffer.AttachCharBlocks(systemReader.BlockAllocator.DetachBlocks());
1432-
}
1433-
else if (reader is PositionAwareStreamReaderDirect directReader)
1434-
{
1435-
logBuffer.AttachCharBlocks(directReader.DetachBlocks());
1436-
}
1429+
// Attach char blocks from the reader to the re-read buffer
1430+
if (reader is PositionAwareStreamReaderSystem systemReader)
1431+
{
1432+
logBuffer.AttachCharBlocks(systemReader.BlockAllocator.DetachBlocks());
1433+
}
1434+
else if (reader is PositionAwareStreamReaderDirect directReader)
1435+
{
1436+
logBuffer.AttachCharBlocks(directReader.DetachBlocks());
1437+
}
14371438

1438-
if (maxLinesCount != logBuffer.LineCount)
1439-
{
1440-
_logger.Warn(CultureInfo.InvariantCulture, "LineCount in buffer differs after re-reading. old={0}, new={1}", maxLinesCount, logBuffer.LineCount);
1441-
}
1439+
if (maxLinesCount != logBuffer.LineCount)
1440+
{
1441+
_logger.Warn(CultureInfo.InvariantCulture, "LineCount in buffer differs after re-reading. old={0}, new={1}", maxLinesCount, logBuffer.LineCount);
1442+
}
14421443

1443-
if (dropCount - logBuffer.PrevBuffersDroppedLinesSum != logBuffer.DroppedLinesCount)
1444-
{
1445-
_logger.Warn(CultureInfo.InvariantCulture, "DroppedLinesCount in buffer differs after re-reading. old={0}, new={1}", logBuffer.DroppedLinesCount, dropCount);
1446-
logBuffer.DroppedLinesCount = dropCount - logBuffer.PrevBuffersDroppedLinesSum;
1444+
if (dropCount - logBuffer.PrevBuffersDroppedLinesSum != logBuffer.DroppedLinesCount)
1445+
{
1446+
_logger.Warn(CultureInfo.InvariantCulture, "DroppedLinesCount in buffer differs after re-reading. old={0}, new={1}", logBuffer.DroppedLinesCount, dropCount);
1447+
logBuffer.DroppedLinesCount = dropCount - logBuffer.PrevBuffersDroppedLinesSum;
1448+
}
14471449
}
14481450

14491451
GC.KeepAlive(fileStream);
14501452
}
1451-
catch (IOException e)
1452-
{
1453-
_logger.Warn(e);
1454-
}
1455-
finally
1456-
{
1457-
(reader as IDisposable)?.Dispose();
1458-
fileStream.Close();
1459-
}
14601453
}
14611454
}
14621455

@@ -2004,7 +1997,7 @@ protected virtual void OnRespawned ()
20041997
#region IBufferPinning
20051998

20061999
/// <inheritdoc />
2007-
PinHandle IBufferPinning.PinRange(int startLine, int endLine)
2000+
PinHandle IBufferPinning.PinRange (int startLine, int endLine)
20082001
{
20092002
using var readLock = BufferIndex.AcquireReadLock();
20102003
return BufferIndex.PinRange(startLine, endLine);

‎src/LogExpert.Tests/Buffers/LogBufferCharBlockTests.cs‎

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -202,8 +202,24 @@ public void BlockBackedLines_ContentSurvivesUntilEviction ()
202202
var retrieved2 = buffer.GetLineMemoryOfBlock(1);
203203
Assert.That(retrieved1.HasValue, Is.True);
204204
Assert.That(retrieved2.HasValue, Is.True);
205-
Assert.That(retrieved1.Value.FullLine.Span.ToString(), Is.EqualTo("Hello World"));
206-
Assert.That(retrieved2.Value.FullLine.Span.ToString(), Is.EqualTo("Second Line"));
205+
206+
if (retrieved1 is not { } retrievedLine1)
207+
{
208+
Assert.Fail("Expected first retrieved line to have a value.");
209+
}
210+
else
211+
{
212+
Assert.That(retrievedLine1.FullLine.Span.ToString(), Is.EqualTo("Hello World"));
213+
}
214+
215+
if (retrieved2 is not { } retrievedLine2)
216+
{
217+
Assert.Fail("Expected second retrieved line to have a value.");
218+
}
219+
else
220+
{
221+
Assert.That(retrievedLine2.FullLine.Span.ToString(), Is.EqualTo("Second Line"));
222+
}
207223

208224
// After eviction, blocks are returned and lines are no longer accessible
209225
buffer.EvictContent();

‎src/LogExpert.Tests/StreamReaderTests/LogStreamReaderTest.cs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -331,6 +331,7 @@ public void TryReadLine_BlockAllocatorHasBlocks_AfterReading ()
331331

332332
while (reader.TryReadLine(out _))
333333
{
334+
// Intentionally empty: consume all lines to advance reader state.
334335
}
335336

336337
// The allocator should have at least 1 block
@@ -346,6 +347,7 @@ public void TryReadLine_DetachBlocks_TransfersOwnership ()
346347

347348
while (reader.TryReadLine(out _))
348349
{
350+
// Intentionally empty: consume all lines to advance reader state.
349351
}
350352

351353
var blocks = reader.BlockAllocator.DetachBlocks();

‎src/LogExpert.Tests/StreamReaderTests/PositionAwareStreamReaderDirectTests.cs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -275,6 +275,7 @@ public void DetachBlocks_ReturnsCompletedBlocks ()
275275
// Read all lines
276276
while (reader.TryReadLine(out _))
277277
{
278+
// Intentionally empty: consume all lines to advance reader state.
278279
}
279280

280281
var blocks = reader.DetachBlocks();

‎src/LogExpert.UI/Controls/LogWindow/LogWindow.cs‎

Lines changed: 36 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6526,8 +6526,25 @@ public IColumnMemory GetCellValue (int rowIndex, int columnIndex)
65266526
: Column.EmptyColumn;
65276527
}
65286528
}
6529-
catch
6529+
catch (IndexOutOfRangeException ex)
65306530
{
6531+
#if DEBUG
6532+
_logger.Warn(ex, "Failed to get cell value due to index error. rowIndex={RowIndex}, columnIndex={ColumnIndex}", rowIndex, columnIndex);
6533+
#endif
6534+
return Column.EmptyColumn;
6535+
}
6536+
catch (ArgumentOutOfRangeException ex)
6537+
{
6538+
#if DEBUG
6539+
_logger.Warn(ex, "Failed to get cell value due to argument range error. rowIndex={RowIndex}, columnIndex={ColumnIndex}", rowIndex, columnIndex);
6540+
#endif
6541+
return Column.EmptyColumn;
6542+
}
6543+
catch (NullReferenceException ex)
6544+
{
6545+
#if DEBUG
6546+
_logger.Warn(ex, "Failed to get cell value due to null state. rowIndex={RowIndex}, columnIndex={ColumnIndex}", rowIndex, columnIndex);
6547+
#endif
65316548
return Column.EmptyColumn;
65326549
}
65336550

@@ -6572,8 +6589,25 @@ private IColumnMemory GetFilterCellValue (int rowIndex, int columnIndex)
65726589
: Column.EmptyColumn;
65736590
}
65746591
}
6575-
catch
6592+
catch (IndexOutOfRangeException ex)
65766593
{
6594+
#if DEBUG
6595+
_logger.Warn(ex, "Failed to get filter cell value due to index error. rowIndex={RowIndex}, columnIndex={ColumnIndex}", rowIndex, columnIndex);
6596+
#endif
6597+
return Column.EmptyColumn;
6598+
}
6599+
catch (ArgumentOutOfRangeException ex)
6600+
{
6601+
#if DEBUG
6602+
_logger.Warn(ex, "Failed to get filter cell value due to argument range error. rowIndex={RowIndex}, columnIndex={ColumnIndex}", rowIndex, columnIndex);
6603+
#endif
6604+
return Column.EmptyColumn;
6605+
}
6606+
catch (NullReferenceException ex)
6607+
{
6608+
#if DEBUG
6609+
_logger.Warn(ex, "Failed to get filter cell value due to null state. rowIndex={RowIndex}, columnIndex={ColumnIndex}", rowIndex, columnIndex);
6610+
#endif
65776611
return Column.EmptyColumn;
65786612
}
65796613

0 commit comments

Comments
 (0)