Synchronize IAnnotationMap/AnnotationModel contract with implementation

- Clarified contract of IAnnotationMap.getLockObject() and
setLockObject().
- Clarified contract of AnnotationModel.getLockObject() and
setLockObject().
- Added warning in AnnotationMap.setLockObject() if the lock is changed
- Removed dead code in AnnotationModel.cleanup because mapLock can't be
null.

Co-authored-by: Andrey Loskutov <[email protected]>

Author: mehmet-karaman

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.
+	 * <p>
+	 * If the lock object is not explicitly set by this method, the annotation map uses its internal
+	 * lock object for synchronization.
+	 * </p>
+	 * 
+	 * <p>
+	 * <em>You should not override an existing lock object unless you own that lock object yourself.
+	 * Use the existing lock object instead.</em>
+	 * </p>
+	 *
+	 * @param lockObject the lock object. If <code>null</code> 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 <b>never</b> returns null
+	 */
 	@Override
 	public synchronized Object getLockObject() {
 		if (fLockObject == null) {

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 <b>never</b> return null. Clients should use the lock
+	 * object in order to synchronize concurrent access to the internal map data.
+	 * 
+	 * @return <b>never</b> 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.
+	 * 
+	 * <p>
+	 * <em>You should not override an existing lock object unless you own that lock object yourself.
+	 * Use the existing lock object instead.</em>
+	 * </p>
+	 *
+	 * @param lockObject the lock object. If <code>null</code> 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<Annotation> 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) {

bundles/org.eclipse.text/src/org/eclipse/jface/text/source/IAnnotationMap.java

@@ -79,4 +79,28 @@ public interface IAnnotationMap extends Map<Annotation, Position>, ISynchronizab
 	 */
 	@Override
 	Collection<Position> values();
+	
+	/**
+	 * Implementers of this interface should <b>never</b> 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 <code>null</code>
+	 */
+	@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.
+	 * <p>
+	 * <em>You should not override an existing lock object unless you own that lock object yourself.
+	 * Use the existing lock object instead.</em>
+	 * </p>
+	 *
+	 * @param lockObject the lock object. Never <code>null</code>.
+	 */
+	@Override
+	void setLockObject(Object lockObject);
 }