Guard access to elements/batchedEntries for MT access

Should avoid duplicated groups created on Error log view creation /
first update running in parallel with entries added to the log.

Fixes eclipse-platform#1245

Author: iloveeclipse

bundles/org.eclipse.ui.views.log/META-INF/MANIFEST.MF

@@ -2,7 +2,7 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-Name: %name
 Bundle-SymbolicName: org.eclipse.ui.views.log;singleton:=true
-Bundle-Version: 1.4.100.qualifier
+Bundle-Version: 1.4.200.qualifier
 Bundle-Activator: org.eclipse.ui.internal.views.log.Activator
 Bundle-Vendor: %provider-name
 Require-Bundle: org.eclipse.core.runtime;bundle-version="[3.29.0,4.0.0)",

bundles/org.eclipse.ui.views.log/src/org/eclipse/ui/internal/views/log/LogView.java

@@ -111,12 +111,12 @@ public class LogView extends ViewPart implements LogListener {
 
 	private ServiceTracker<LogReaderService, LogReaderService> logReaderServiceTracker;
 
-	private List<AbstractEntry> elements;
+	private final List<AbstractEntry> elements;
 	private Map<Object, Group> groups;
 	private LogSession currentSession;
 
-	private List<LogEntry> batchedEntries;
-	private boolean batchEntries;
+	private final List<LogEntry> batchedEntries;
+	private volatile boolean batchEntries;
 
 	private Clipboard fClipboard;
 
@@ -244,10 +244,11 @@ public void perspectiveChanged(IWorkbenchPage page, IPerspectiveDescriptor persp
 
 				if (part.equals(LogView.this)) {
 					if (changeId.equals(IWorkbenchPage.CHANGE_VIEW_SHOW)) {
-						if (!batchedEntries.isEmpty()) {
-							pushBatchedEntries();
+						synchronized (batchedEntries) {
+							if (!batchedEntries.isEmpty()) {
+								pushBatchedEntries();
+							}
 						}
-
 						batchEntries = false;
 					} else if (changeId.equals(IWorkbenchPage.CHANGE_VIEW_HIDE)) {
 						batchEntries = true;
@@ -834,10 +835,12 @@ public AbstractEntry[] getElements() {
 
 	public void handleClear() {
 		BusyIndicator.showWhile(fTree.getDisplay(), () -> {
-			elements.clear();
-			groups.clear();
-			if (currentSession != null) {
-				currentSession.removeAllChildren();
+			synchronized (elements) {
+				elements.clear();
+				groups.clear();
+				if (currentSession != null) {
+					currentSession.removeAllChildren();
+				}
 			}
 			asyncRefresh(false);
 			resetDialogButtons();
@@ -888,10 +891,12 @@ private CompletableFuture<List<LogEntry>> fetchLogEntries() {
 	}
 
 	private void updateLogViewer(List<LogEntry> entries) {
-		elements.clear();
-		groups.clear();
-		group(entries);
-		limitEntriesCount();
+		synchronized (elements) {
+			elements.clear();
+			groups.clear();
+			group(entries);
+			limitEntriesCount();
+		}
 		setContentDescription(getTitleSummary());
 
 		asyncRefresh(false);
@@ -1082,7 +1087,9 @@ public void logged(org.osgi.service.log.LogEntry input) {
 		if (batchEntries) {
 			// create LogEntry immediately to don't loose IStatus creation date.
 			LogEntry entry = betterInput != null ? createLogEntry(betterInput) : createLogEntry(input);
-			batchedEntries.add(entry);
+			synchronized (batchedEntries) {
+				batchedEntries.add(entry);
+			}
 			return;
 		}
 
@@ -1093,11 +1100,16 @@ public void logged(org.osgi.service.log.LogEntry input) {
 		} else {
 			LogEntry entry = betterInput != null ? createLogEntry(betterInput) : createLogEntry(input);
 
-			if (!batchedEntries.isEmpty()) {
+			boolean useBatchedEntries;
+			synchronized (batchedEntries) {
+				useBatchedEntries = !batchedEntries.isEmpty();
 				// batch new entry as well, to have only one asyncRefresh()
-				batchedEntries.add(entry);
-				pushBatchedEntries();
-			} else {
+				if (useBatchedEntries) {
+					batchedEntries.add(entry);
+					pushBatchedEntries();
+				}
+			}
+			if (!useBatchedEntries) {
 				pushEntry(entry);
 				asyncRefresh(true);
 			}
@@ -1111,11 +1123,14 @@ private void pushBatchedEntries() {
 		Job job = new Job(Messages.LogView_AddingBatchedEvents) {
 			@Override
 			protected IStatus run(IProgressMonitor monitor) {
-				for (int i = 0; i < batchedEntries.size(); i++) {
-					if (!monitor.isCanceled()) {
-						LogEntry entry = batchedEntries.get(i);
-						pushEntry(entry);
-						batchedEntries.remove(i);
+				synchronized (batchedEntries) {
+					Iterator<LogEntry> iterator = batchedEntries.iterator();
+					while (iterator.hasNext()) {
+						if (!monitor.isCanceled()) {
+							LogEntry entry = iterator.next();
+							pushEntry(entry);
+							iterator.remove();
+						}
 					}
 				}
 				asyncRefresh(true);
@@ -1159,10 +1174,12 @@ private LogEntry createLogEntry(FrameworkLogEntry input) {
 		return logEntry;
 	}
 
-	private synchronized void pushEntry(LogEntry entry) {
-		if (LogReader.isLogged(entry, fMemento)) {
-			group(Collections.singletonList(entry));
-			limitEntriesCount();
+	private void pushEntry(LogEntry entry) {
+		synchronized (elements) {
+			if (LogReader.isLogged(entry, fMemento)) {
+				group(Collections.singletonList(entry));
+				limitEntriesCount();
+			}
 		}
 		asyncRefresh(true);
 	}
@@ -1558,6 +1575,7 @@ private void setComparator(byte sortType) {
 					date2 = ((LogSession) e2).getDate() == null ? 0 : ((LogSession) e2).getDate().getTime();
 				}
 				if (date1 == date2) {
+					// XXX: this breaks stable sort, as elements is mutable
 					int result = elements.indexOf(e2) - elements.indexOf(e1);
 					if (DATE_ORDER == DESCENDING)
 						result *= DESCENDING;
@@ -1653,6 +1671,8 @@ public int compare(Viewer viewer, Object e1, Object e2) {
 							// i.e. latest log message first, therefore index(e2)-index(e1)
 							result = indexOf(children, e2) - indexOf(children, e1);
 						} else {
+							// XXX: this breaks stable sort, as elements is
+							// mutable
 							result = elements.indexOf(e1) - elements.indexOf(e2);
 						}
 						if (DATE_ORDER == DESCENDING)