From a3371d2d7ba6501fc1346aca2f18a311167ec2e3 Mon Sep 17 00:00:00 2001 From: Axel Howind Date: Tue, 24 Dec 2024 07:02:25 +0100 Subject: [PATCH] fix race condition in LogEntriesObservableList --- .../utility/fx/LogEntriesObservableList.java | 32 +++++++++------- .../com/dua3/utility/logging/LogBuffer.java | 37 +++++++++++++++++++ 2 files changed, 56 insertions(+), 13 deletions(-) diff --git a/utility-fx/src/main/java/com/dua3/utility/fx/LogEntriesObservableList.java b/utility-fx/src/main/java/com/dua3/utility/fx/LogEntriesObservableList.java index acaf0925..a0b7352a 100644 --- a/utility-fx/src/main/java/com/dua3/utility/fx/LogEntriesObservableList.java +++ b/utility-fx/src/main/java/com/dua3/utility/fx/LogEntriesObservableList.java @@ -10,7 +10,7 @@ import java.util.Arrays; import java.util.Collections; import java.util.List; -import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.locks.Condition; import java.util.concurrent.locks.Lock; import java.util.concurrent.locks.ReadWriteLock; @@ -25,7 +25,8 @@ final class LogEntriesObservableList extends ObservableListBase implem private static final long REST_TIME_IN_MS = 10; private volatile List data = Collections.emptyList(); - private final AtomicInteger queuedRemoves = new AtomicInteger(); + private final AtomicLong totalAdded = new AtomicLong(0); + private final AtomicLong totalRemoved = new AtomicLong(0); private final ReadWriteLock updateLock = new ReentrantReadWriteLock(); private final Lock updateWriteLock = updateLock.writeLock(); @@ -46,18 +47,25 @@ final class LogEntriesObservableList extends ObservableListBase implem try { updatesAvailableCondition.await(); - List newData = Arrays.asList(buffer.toArray()); - Platform.runLater(() -> { - int newSz = newData.size(); - int oldSz = data.size(); - int removedRows = queuedRemoves.getAndSet(0); - int remainingRows = oldSz - removedRows; - int addedRows = newSz - remainingRows; - try { beginChange(); - List removed = List.copyOf(data.subList(0, removedRows)); + LogBuffer.BufferState state = buffer.getBufferState(); + List newData = Arrays.asList(state.entries()); + totalAdded.getAndSet(state.totalAdded()); + long ta = totalAdded.get(); + long trOld = totalRemoved.getAndSet(state.totalRemoved()); + long tr = totalRemoved.get(); + + assert newData.size() == ta - tr; + + int newSz = newData.size(); + int oldSz = data.size(); + int removedRows = (int) Math.min(oldSz, (tr - trOld)); + int remainingRows = oldSz - removedRows; + int addedRows = newSz - remainingRows; + List removed = List.copyOf(data.subList(0, removedRows)); + data = newData; nextRemove(0, removed); nextAdd(newSz - addedRows, newSz); @@ -102,7 +110,6 @@ public LogEntry get(int idx) { public void entries(int removed, int added) { updateWriteLock.lock(); try { - queuedRemoves.addAndGet(removed); updatesAvailableCondition.signalAll(); } finally { updateWriteLock.unlock(); @@ -113,7 +120,6 @@ public void entries(int removed, int added) { public void clear() { updateWriteLock.lock(); try { - queuedRemoves.set(data.size()); updatesAvailableCondition.signalAll(); } finally { updateWriteLock.unlock(); diff --git a/utility-logging/src/main/java/com/dua3/utility/logging/LogBuffer.java b/utility-logging/src/main/java/com/dua3/utility/logging/LogBuffer.java index ca81835b..c3a6b5b0 100644 --- a/utility-logging/src/main/java/com/dua3/utility/logging/LogBuffer.java +++ b/utility-logging/src/main/java/com/dua3/utility/logging/LogBuffer.java @@ -9,6 +9,7 @@ import java.util.ArrayList; import java.util.Collection; import java.util.List; +import java.util.concurrent.atomic.AtomicLong; /** * A log buffer class intended to provide a buffer for log messages to display in GUI applications. @@ -22,6 +23,8 @@ public class LogBuffer implements LogEntryHandler, Externalizable { private final RingBuffer buffer; private final Collection listeners = new ArrayList<>(); + private final AtomicLong totalAdded = new AtomicLong(0); + private final AtomicLong totalRemoved = new AtomicLong(0); /** * Construct a new LogBuffer instance with default capacity. @@ -81,6 +84,8 @@ public void handleEntry(LogEntry entry) { int removed; synchronized (buffer) { removed = buffer.put(entry) ? 0 : 1; + this.totalAdded.incrementAndGet(); + this.totalRemoved.addAndGet(removed); } listeners.forEach(listener -> listener.entries(removed, 1)); } @@ -93,6 +98,7 @@ public void handleEntry(LogEntry entry) { public void clear() { synchronized (listeners) { synchronized (buffer) { + totalRemoved.addAndGet(buffer.size()); buffer.clear(); } listeners.forEach(LogBufferListener::clear); @@ -110,6 +116,37 @@ public LogEntry[] toArray() { } } + /** + * Represents the state of a buffer. + * + * This record is used to encapsulate the current state of a LogBuffer, + * including its entries, the total number of log entries that have been + * removed, and the total number of log entries that have been added. + * + * @param entries the array of LogEntry objects currently in the buffer + * @param totalRemoved the total count of log entries that have been removed from the buffer + * @param totalAdded the total count of log entries that have been added to the buffer + */ + public record BufferState(LogEntry[] entries, long totalRemoved, long totalAdded) {} + + /** + * Retrieves the current state of the buffer, encapsulating the entries within the buffer, + * the total number of entries removed, and the total number of entries added. + * This method is thread-safe as it synchronizes on the buffer while performing operations. + * + * @return a {@code BufferState} instance containing the current buffer entries, + * total removed entries, and total added entries + */ + public BufferState getBufferState() { + synchronized (buffer) { + LogEntry[] array = toArray(); + long r = totalRemoved.get(); + long a = totalAdded.get(); + assert array.length == a - r; + return new BufferState(array, r, a); + } + } + /** * Get the LogEntry at the specified index in the LogBuffer. *