diff --git a/bundles/org.eclipse.text/src/org/eclipse/jface/text/source/AnnotationMap.java b/bundles/org.eclipse.text/src/org/eclipse/jface/text/source/AnnotationMap.java index 23459298044..e27c9a9e129 100644 --- a/bundles/org.eclipse.text/src/org/eclipse/jface/text/source/AnnotationMap.java +++ b/bundles/org.eclipse.text/src/org/eclipse/jface/text/source/AnnotationMap.java @@ -21,6 +21,8 @@ import java.util.Map; import java.util.Set; +import org.eclipse.core.runtime.ILog; + import org.eclipse.jface.text.Position; @@ -54,11 +56,38 @@ public AnnotationMap(int capacity) { fInternalMap= new HashMap<>(capacity); } + /** + * Sets the lock object for this object. Subsequent calls to specified methods of this object + * are synchronized on this lock object. Which methods are synchronized is specified by the + * implementer. + *

+ * If the lock object is not explicitly set by this method, the annotation map uses its internal + * lock object for synchronization. + *

+ * + *

+ * You should not override an existing lock object unless you own that lock object yourself. + * Use the existing lock object instead. + *

+ * + * @param lockObject the lock object. If null is given, internal lock object is + * used for locking. + */ @Override public synchronized void setLockObject(Object lockObject) { + if(fLockObject != null && fLockObject != lockObject) { + ILog log= ILog.of(AnnotationMap.class); + String message = "Attempt to override existing lock object of AnnotationMap."; //$NON-NLS-1$ + log.warn(message, new IllegalStateException(message)); + } fLockObject= lockObject; } + /** + * Lock object used to synchronize concurrent access to the internal map data. + * + * @return never returns null + */ @Override public synchronized Object getLockObject() { if (fLockObject == null) { diff --git a/bundles/org.eclipse.text/src/org/eclipse/jface/text/source/AnnotationModel.java b/bundles/org.eclipse.text/src/org/eclipse/jface/text/source/AnnotationModel.java index 5ab6cf9e0dc..8e90654a28c 100644 --- a/bundles/org.eclipse.text/src/org/eclipse/jface/text/source/AnnotationModel.java +++ b/bundles/org.eclipse.text/src/org/eclipse/jface/text/source/AnnotationModel.java @@ -23,6 +23,7 @@ import java.util.Map; import java.util.Map.Entry; import java.util.NoSuchElementException; +import java.util.Objects; import java.util.concurrent.ConcurrentHashMap; import org.eclipse.core.runtime.Assert; @@ -338,11 +339,30 @@ protected IAnnotationMap getAnnotationMap() { return (IAnnotationMap) fAnnotations; } + /** + * Subclasses should never return null. Clients should use the lock + * object in order to synchronize concurrent access to the internal map data. + * + * @return never returns null + */ @Override public Object getLockObject() { - return getAnnotationMap().getLockObject(); + return Objects.requireNonNull(getAnnotationMap().getLockObject()); } + /** + * Sets the lock object for this object. Subsequent calls to specified methods of this object + * are synchronized on this lock object. Which methods are synchronized is specified by the + * implementer. + * + *

+ * You should not override an existing lock object unless you own that lock object yourself. + * Use the existing lock object instead. + *

+ * + * @param lockObject the lock object. If null is given, default implementation uses + * the internal lock object for locking. + */ @Override public void setLockObject(Object lockObject) { getAnnotationMap().setLockObject(lockObject); @@ -661,23 +681,12 @@ private void cleanup(boolean fireModelChanged, boolean forkNotification) { IAnnotationMap annotations= getAnnotationMap(); Object mapLock = annotations.getLockObject(); - if (mapLock == null) { - Iterator e= annotations.keySetIterator(); - while (e.hasNext()) { - Annotation a= e.next(); - Position p= annotations.get(a); + synchronized (mapLock) { + annotations.forEach((a, p) -> { if (p == null || p.isDeleted()) { deleted.add(a); } - } - } else { - synchronized (mapLock) { - annotations.forEach((a, p) -> { - if (p == null || p.isDeleted()) { - deleted.add(a); - } - }); - } + }); } if (fireModelChanged && forkNotification) { diff --git a/bundles/org.eclipse.text/src/org/eclipse/jface/text/source/IAnnotationMap.java b/bundles/org.eclipse.text/src/org/eclipse/jface/text/source/IAnnotationMap.java index 6f4d2b04bef..debc02ebf49 100644 --- a/bundles/org.eclipse.text/src/org/eclipse/jface/text/source/IAnnotationMap.java +++ b/bundles/org.eclipse.text/src/org/eclipse/jface/text/source/IAnnotationMap.java @@ -79,4 +79,28 @@ public interface IAnnotationMap extends Map, ISynchronizab */ @Override Collection values(); + + /** + * Implementers of this interface should never return null. Clients should use the lock + * object in order to synchronize concurrent access to the implementer. + * + * @return the lock object to be used, if no explicit lock object is set, internal one should + * be used, therefore never null + */ + @Override + Object getLockObject(); + + /** + * Sets the lock object for this object. Subsequent calls to specified methods of this object + * are synchronized on this lock object. Which methods are synchronized is specified by the + * implementer. + *

+ * You should not override an existing lock object unless you own that lock object yourself. + * Use the existing lock object instead. + *

+ * + * @param lockObject the lock object. Never null. + */ + @Override + void setLockObject(Object lockObject); }