Bug 292851. Protection against concurrent access to the shared AST between CEditor and refactorings.

core/org.eclipse.cdt.core/model/org/eclipse/cdt/internal/core/model/ASTCache.java

@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2007, 2009 Wind River Systems, Inc. and others. All rights reserved.
+ * Copyright (c) 2007, 2011 Wind River Systems, Inc. and others. All rights reserved.
  * This program and the accompanying materials are made available under the
  * terms of the Eclipse Public License v1.0 which accompanies this distribution,
  * and is available at http://www.eclipse.org/legal/epl-v10.html
@@ -7,9 +7,12 @@
  * Contributors: 
  *     Anton Leherbauer (Wind River Systems) - initial API and implementation
  *     Markus Schorn (Wind River Systems)
+ *     Sergey Prigogin (Google)
  ******************************************************************************/
 package org.eclipse.cdt.internal.core.model;
 
+import java.util.concurrent.Semaphore;
+
 import org.eclipse.cdt.core.CCorePlugin;
 import org.eclipse.cdt.core.dom.ast.IASTTranslationUnit;
 import org.eclipse.cdt.core.index.IIndex;
@@ -71,6 +74,11 @@ public class ASTCache {
 	private ITranslationUnit fActiveTU;
 	/** The cached AST if any */
 	private IASTTranslationUnit fAST;
+	/**
+	 * The semaphore controlling exclusive access to the cached AST.
+	 * <code>null</code> when fAST is <code>null</code>.
+	 */
+	private Semaphore fASTSemaphore;
 	/** 
 	 * The timestamp of the last index write access at the time
 	 * the AST got cached. A cached AST becomes invalid on any index
@@ -212,13 +220,15 @@ public class ASTCache {
 		}
 		
 		try {
-			IASTTranslationUnit ast= getAST(tUnit, index, wait, monitor);
+			IASTTranslationUnit ast= acquireSharedAST(tUnit, index, wait, monitor);
 			ILanguage lang= (tUnit instanceof TranslationUnit) ? ((TranslationUnit) tUnit).getLanguageOfContext() : tUnit.getLanguage();
 			if (ast == null) {
 				return astRunnable.runOnAST(lang, ast);
 			}
-			synchronized (ast) {
+			try {
 				return astRunnable.runOnAST(lang, ast);
+			} finally {
+				releaseSharedAST(ast);
 			}
 		} catch (CoreException e) {
 			return e.getStatus();
@@ -227,6 +237,31 @@ public class ASTCache {
 		}
 	}
 
+	public IASTTranslationUnit acquireSharedAST(ITranslationUnit tUnit, IIndex index, boolean wait,
+			IProgressMonitor monitor) {
+		IASTTranslationUnit ast= getAST(tUnit, index, wait, monitor);
+		if (ast == null)
+			return null;
+		synchronized (fCacheMutex) {
+			if (ast == fAST) {
+				try {
+					fASTSemaphore.acquire();
+				} catch (InterruptedException e) {
+					throw new OperationCanceledException();
+				}
+			}
+		}
+		return ast;
+	}
+
+	public void releaseSharedAST(IASTTranslationUnit ast) {
+		synchronized (fCacheMutex) {
+			if (ast == fAST) {
+				fASTSemaphore.release();
+			}
+		}
+	}
+
 	/**
 	 * Caches the given AST for the given translation unit.
 	 *
@@ -248,6 +283,7 @@ public class ASTCache {
 			disposeAST();
 
 		fAST= ast;
+		fASTSemaphore= fAST == null ? null : new Semaphore(1); 
 		fLastWriteOnIndex= fAST == null ? 0 : fAST.getIndex().getLastWriteAccess();
 
 		// Signal AST change
@@ -266,6 +302,7 @@ public class ASTCache {
 				System.out.println(DEBUG_PREFIX + getThreadName() + "disposing AST: " + toString(fAST) + " for: " + toString(fActiveTU)); //$NON-NLS-1$ //$NON-NLS-2$ 
 	
 			fAST= null;
+			fASTSemaphore= null;
 			cache(null, null);
 		}
 	}

core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/editor/ASTProvider.java

@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2000, 2008 IBM Corporation and others.
+ * Copyright (c) 2000, 2011 IBM Corporation and others.
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Eclipse Public License v1.0
  * which accompanies this distribution, and is available at
@@ -9,6 +9,7 @@
  *     IBM Corporation - initial API and implementation
  *     Anton Leherbauer (Wind River Systems) - Adapted for CDT
  *     Markus Schorn (Wind River Systems)
+ *     Sergey Prigogin (Google)
  *******************************************************************************/
 package org.eclipse.cdt.internal.ui.editor;
 
@@ -28,13 +29,13 @@ import org.eclipse.ui.PlatformUI;
 import org.eclipse.ui.texteditor.ITextEditor;
 
 import org.eclipse.cdt.core.dom.ast.IASTTranslationUnit;
+import org.eclipse.cdt.core.index.IIndex;
 import org.eclipse.cdt.core.model.ICElement;
 import org.eclipse.cdt.core.model.ITranslationUnit;
 import org.eclipse.cdt.ui.CUIPlugin;
 
 import org.eclipse.cdt.internal.core.model.ASTCache;
 
-
 /**
  * Provides a shared AST for clients. The shared AST is
  * the AST of the active CEditor's input element.
@@ -42,7 +43,6 @@ import org.eclipse.cdt.internal.core.model.ASTCache;
  * @since 4.0
  */
 public final class ASTProvider {
-
 	/**
 	 * Wait flag.
 	 */
@@ -329,16 +329,57 @@ public final class ASTProvider {
 			ASTCache.ASTRunnable astRunnable) {
 		Assert.isTrue(cElement instanceof ITranslationUnit);
 		final ITranslationUnit tu = (ITranslationUnit) cElement;
-		if (!tu.isOpen())
+		if (!canUseCache(tu, waitFlag))
 			return Status.CANCEL_STATUS;
+		return fCache.runOnAST(tu, waitFlag != WAIT_NO, monitor, astRunnable);
+	}
 
+	/**
+	 * Returns a shared AST and locks it for exclusive access. An AST obtained from this
+	 * method has to be released by calling {@link #releaseSharedAST(IASTTranslationUnit)}.
+	 * Subsequent call to this method will block until the AST is released.
+	 * <p>
+	 * The AST can be released by a thread other than the one that acquired it.
+	 * <p>
+	 * An index lock must be held by the caller when calling this method. The index lock may
+	 * not be released until the AST is released.
+	 * 
+	 * @param tu The translation unit to get the AST for.
+	 * @param index index with read lock held.
+	 * @param waitFlag condition for waiting for the AST to be built.
+	 * @param monitor a progress monitor, may be <code>null</code>.
+	 * @return the shared AST, or <code>null</code> if the shared AST is not available.
+	 */
+	public final IASTTranslationUnit acquireSharedAST(ITranslationUnit tu, IIndex index,
+			WAIT_FLAG waitFlag, IProgressMonitor monitor) {
+		if (!canUseCache(tu, waitFlag))
+			return null;
+		return fCache.acquireSharedAST(tu, index, waitFlag != WAIT_NO, monitor);
+	}
+
+	/**
+	 * Releases a shared AST previously acquired by calling
+	 * {@link #acquireSharedAST(ITranslationUnit, IIndex, WAIT_FLAG, IProgressMonitor)}.
+	 * <p>
+	 * Can be called by a thread other than the one that acquired the AST.
+	*
+	 * @param ast the AST to release.
+	 */
+	public final void releaseSharedAST(IASTTranslationUnit ast) {
+		fCache.releaseSharedAST(ast);
+	}
+
+	private synchronized boolean canUseCache(ITranslationUnit tu, WAIT_FLAG waitFlag) {
 		final boolean isActive= fCache.isActiveElement(tu);
+		if (!tu.isOpen())
+			return false;
+
 		if (waitFlag == WAIT_ACTIVE_ONLY && !isActive) {
-			return Status.CANCEL_STATUS;
+			return false;
 		}
 		if (isActive && updateModificationStamp()) {
 			fCache.disposeAST();
 		}
-		return fCache.runOnAST(tu, waitFlag != WAIT_NO, monitor, astRunnable);
+		return true;
 	}
 }

core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/refactoring/RefactoringASTCache.java

@@ -16,9 +16,7 @@ import java.util.concurrent.ConcurrentHashMap;
 import org.eclipse.core.runtime.Assert;
 import org.eclipse.core.runtime.CoreException;
 import org.eclipse.core.runtime.IProgressMonitor;
-import org.eclipse.core.runtime.IStatus;
 import org.eclipse.core.runtime.OperationCanceledException;
-import org.eclipse.core.runtime.Status;
 import org.eclipse.ui.services.IDisposable;
 
 import org.eclipse.cdt.core.CCorePlugin;
@@ -26,11 +24,9 @@ import org.eclipse.cdt.core.dom.ast.IASTTranslationUnit;
 import org.eclipse.cdt.core.index.IIndex;
 import org.eclipse.cdt.core.model.CoreModel;
 import org.eclipse.cdt.core.model.ICProject;
-import org.eclipse.cdt.core.model.ILanguage;
 import org.eclipse.cdt.core.model.ITranslationUnit;
 import org.eclipse.cdt.ui.CUIPlugin;
 
-import org.eclipse.cdt.internal.core.model.ASTCache.ASTRunnable;
 import org.eclipse.cdt.internal.corext.util.CModelUtil;
 
 import org.eclipse.cdt.internal.ui.editor.ASTProvider;
@@ -39,8 +35,8 @@ import org.eclipse.cdt.internal.ui.editor.ASTProvider;
  * Cache containing ASTs for the translation units participating in refactoring.
  * The cache object has to be disposed of after use. Failure to do so may cause
  * loss of index lock.
- * 
+ * <p>
- * This class is thread-safe.
+ * This class is not thread-safe.
  */
 public class RefactoringASTCache implements IDisposable {
 	private static final int PARSE_MODE = ITranslationUnit.AST_SKIP_ALL_HEADERS
@@ -49,21 +45,24 @@ public class RefactoringASTCache implements IDisposable {
 			| ITranslationUnit.AST_PARSE_INACTIVE_CODE;
 
 	private final Map<ITranslationUnit, IASTTranslationUnit> fASTCache;
-	private final Object astBuildMutex;
 	private IIndex fIndex;
+	private IASTTranslationUnit fSharedAST;
 	private boolean fDisposed;
 
 	public RefactoringASTCache() {
 		fASTCache = new ConcurrentHashMap<ITranslationUnit, IASTTranslationUnit>();
-		astBuildMutex = new Object();
 	}
 
 	/**
 	 * Returns an AST for the given translation unit. The AST is built for the working
 	 * copy of the translation unit if such working copy exists. The returned AST is
 	 * a shared one whenever possible.
-	 * NOTE: No references to the AST or its nodes can be kept after calling
+	 * <p>
+	 * An AST returned by this method should not be accessed concurrently by multiple threads.
+	 * <p>
+	 * <b>NOTE</b>: No references to the AST or its nodes can be kept after calling
 	 * the {@link #dispose()} method.
+	 *
 	 * @param tu The translation unit.
 	 * @param pm A progress monitor.
 	 * @return An AST, or <code>null</code> if the AST cannot be obtained.
@@ -76,38 +75,31 @@ public class RefactoringASTCache implements IDisposable {
 			throw new OperationCanceledException();
 
 		tu= CModelUtil.toWorkingCopy(tu);
-    	IASTTranslationUnit ast;
+    	// Try to get a shared AST before creating our own.
-		ast= fASTCache.get(tu);
+    	IASTTranslationUnit ast= fASTCache.get(tu);
-
+    	if (ast == null) {
-        if (ast == null) {
+    		if (fSharedAST != null && tu.equals(fSharedAST.getOriginatingTranslationUnit())) {
-        	// Try to get a shared AST before creating our own.
+    			ast = fSharedAST;
-        	final IASTTranslationUnit[] astHolder = new IASTTranslationUnit[1];
+    		} else {
-			ASTProvider.getASTProvider().runOnAST(tu, ASTProvider.WAIT_ACTIVE_ONLY, pm, new ASTRunnable() {
+	        	ast = ASTProvider.getASTProvider().acquireSharedAST(tu, fIndex,
-				public IStatus runOnAST(ILanguage lang, IASTTranslationUnit ast) throws CoreException {
+	        			ASTProvider.WAIT_ACTIVE_ONLY, pm);
-					// Leaking of AST outside of runOnAST method is dangerous, but it does not cause
+	        	if (ast == null) {
-					// harm here since the index remains locked for the duration of the AST life span.
+					if (pm != null && pm.isCanceled())
-					astHolder[0] = ast;
+						throw new OperationCanceledException();
-					return Status.OK_STATUS;
+					ast= tu.getAST(fIndex, PARSE_MODE);
-				}
+		        	fASTCache.put(tu, ast);
-			});
+	        	} else {
-			ast = astHolder[0];
+	        		if (fSharedAST != null) {
-
+	        			ASTProvider.getASTProvider().releaseSharedAST(fSharedAST);
-        	if (ast == null) {
+	        		}
-				synchronized (astBuildMutex) {
+	        		fSharedAST = ast;
-					ast= fASTCache.get(tu);
+	        	}
-					if (ast == null) {
+    		}
-						if (pm != null && pm.isCanceled())
+    	}
-							throw new OperationCanceledException();
-						ast= tu.getAST(fIndex, PARSE_MODE);
-		            	fASTCache.put(tu, ast);
-					}
-				}
-			}
-        }
         if (pm != null) {
         	pm.done();
         }
-        return ast;
+       	return ast;
     }
 
 	/**
@@ -115,7 +107,7 @@ public class RefactoringASTCache implements IDisposable {
 	 * 
 	 * @return The index.
 	 */
-	public synchronized IIndex getIndex() throws CoreException, OperationCanceledException {
+	public IIndex getIndex() throws CoreException, OperationCanceledException {
         Assert.isTrue(!fDisposed, "RefactoringASTCache is already disposed"); //$NON-NLS-1$
 		if (fIndex == null) {
 			ICProject[] projects = CoreModel.getDefault().getCModel().getCProjects();
@@ -137,6 +129,9 @@ public class RefactoringASTCache implements IDisposable {
 	public void dispose() {
         Assert.isTrue(!fDisposed, "RefactoringASTCache.dispose() called more than once"); //$NON-NLS-1$
 		fDisposed = true;
+		if (fSharedAST != null) {
+			ASTProvider.getASTProvider().releaseSharedAST(fSharedAST);
+		}
 		if (fIndex != null) {
 			fIndex.releaseReadLock();
 		}