From ab9417925c3896bd77aac51f1e29e52407df7c16 Mon Sep 17 00:00:00 2001 From: Sergey Prigogin Date: Wed, 13 Apr 2011 21:38:50 +0000 Subject: [PATCH] Fixed a deadlock. --- .../cdt/internal/core/model/ASTCache.java | 80 +++++++++++-------- .../core/dom/parser/ASTTranslationUnit.java | 15 ++++ .../cdt/internal/ui/editor/ASTProvider.java | 27 ++++++- 3 files changed, 85 insertions(+), 37 deletions(-) diff --git a/core/org.eclipse.cdt.core/model/org/eclipse/cdt/internal/core/model/ASTCache.java b/core/org.eclipse.cdt.core/model/org/eclipse/cdt/internal/core/model/ASTCache.java index 69c189d8098..3068febd2e0 100644 --- a/core/org.eclipse.cdt.core/model/org/eclipse/cdt/internal/core/model/ASTCache.java +++ b/core/org.eclipse.cdt.core/model/org/eclipse/cdt/internal/core/model/ASTCache.java @@ -11,14 +11,13 @@ ******************************************************************************/ 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; import org.eclipse.cdt.core.index.IIndexManager; import org.eclipse.cdt.core.model.ILanguage; import org.eclipse.cdt.core.model.ITranslationUnit; +import org.eclipse.cdt.internal.core.dom.parser.ASTTranslationUnit; import org.eclipse.core.runtime.CoreException; import org.eclipse.core.runtime.IProgressMonitor; import org.eclipse.core.runtime.ISafeRunnable; @@ -74,11 +73,6 @@ public class ASTCache { private ITranslationUnit fActiveTU; /** The cached AST if any */ private IASTTranslationUnit fAST; - /** - * The semaphore controlling exclusive access to the cached AST. - * null when fAST is null. - */ - 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 @@ -96,12 +90,11 @@ public class ASTCache { } /** - * Returns a shared translation unit AST for the given - * translation unit. + * Returns a shared translation unit AST for the given translation unit. *

- * Clients are not allowed to modify the AST and must - * hold a read lock prior to calling this method and continue - * to hold the lock as long as the AST is being used. + * Clients are not allowed to modify the AST and must hold an index read + * lock prior to calling this method and continue to hold the lock as long + * as the AST is being used. *

* * @param tUnit the translation unit @@ -110,7 +103,8 @@ public class ASTCache { * @param progressMonitor the progress monitor or null * @return the AST or null if the AST is not available */ - private IASTTranslationUnit getAST(ITranslationUnit tUnit, IIndex index, boolean wait, IProgressMonitor progressMonitor) { + private IASTTranslationUnit getAST(ITranslationUnit tUnit, IIndex index, boolean wait, + IProgressMonitor progressMonitor) { if (tUnit == null) return null; @@ -237,35 +231,55 @@ 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(); - } + /** + * Returns a shared AST for the given translation unit 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. + *

+ * The AST can be released by a thread other than the one that acquired it. + *

+ * Clients are not allowed to modify the AST and must hold an index read + * lock prior to calling this method and continue to hold the lock as long + * as the AST is being used. + *

+ * + * @param tUnit the translation unit + * @param index the index used to create the AST, needs to be read-locked + * @param wait if true, wait for AST to be computed + * (might compute a new AST) + * @param progressMonitor the progress monitor or null + * @return the AST or null if the AST is not available + */ + public final IASTTranslationUnit acquireSharedAST(ITranslationUnit tUnit, IIndex index, + boolean wait, IProgressMonitor progressMonitor) { + IASTTranslationUnit ast= getAST(tUnit, index, wait, progressMonitor); + if (ast != null) { + try { + ((ASTTranslationUnit) ast).beginExclusiveAccess(); + } catch (InterruptedException e) { + throw new OperationCanceledException(); } } return ast; } - public void releaseSharedAST(IASTTranslationUnit ast) { - synchronized (fCacheMutex) { - if (ast == fAST) { - fASTSemaphore.release(); - } - } + /** + * Releases a shared AST previously acquired by calling + * {@link #acquireSharedAST(ITranslationUnit, IIndex, boolean, IProgressMonitor)}. + *

+ * 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) { + ((ASTTranslationUnit) ast).endExclusiveAccess(); } /** * Caches the given AST for the given translation unit. * - * @param ast the AST + * @param ast the AST * @param tUnit the translation unit */ private void cache(IASTTranslationUnit ast, ITranslationUnit tUnit) { @@ -283,7 +297,6 @@ public class ASTCache { disposeAST(); fAST= ast; - fASTSemaphore= fAST == null ? null : new Semaphore(1); fLastWriteOnIndex= fAST == null ? 0 : fAST.getIndex().getLastWriteAccess(); // Signal AST change @@ -302,7 +315,6 @@ 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); } } diff --git a/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/ASTTranslationUnit.java b/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/ASTTranslationUnit.java index 5c8a8b37357..63d2265bc31 100644 --- a/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/ASTTranslationUnit.java +++ b/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/ASTTranslationUnit.java @@ -11,6 +11,7 @@ package org.eclipse.cdt.internal.core.dom.parser; import java.util.List; +import java.util.concurrent.Semaphore; import org.eclipse.cdt.core.CCorePlugin; import org.eclipse.cdt.core.dom.IName; @@ -70,6 +71,8 @@ public abstract class ASTTranslationUnit extends ASTNode implements IASTTranslat private INodeFactory fNodeFactory; private boolean fForContentAssist; private ITranslationUnit fOriginatingTranslationUnit; + /** The semaphore controlling exclusive access to the AST. */ + private final Semaphore fSemaphore= new Semaphore(1); @Override public final IASTTranslationUnit getTranslationUnit() { @@ -446,4 +449,16 @@ public abstract class ASTTranslationUnit extends ASTNode implements IASTTranslat public void setOriginatingTranslationUnit(ITranslationUnit tu) { this.fOriginatingTranslationUnit = tu; } + + /** + * Starts exclusive access + * @throws InterruptedException + */ + public void beginExclusiveAccess() throws InterruptedException { + fSemaphore.acquire(); + } + + public void endExclusiveAccess() { + fSemaphore.release(); + } } diff --git a/core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/editor/ASTProvider.java b/core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/editor/ASTProvider.java index 7a0eb580249..7401c0b8e98 100644 --- a/core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/editor/ASTProvider.java +++ b/core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/editor/ASTProvider.java @@ -31,10 +31,12 @@ 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.ILanguage; import org.eclipse.cdt.core.model.ITranslationUnit; import org.eclipse.cdt.ui.CUIPlugin; import org.eclipse.cdt.internal.core.model.ASTCache; +import org.eclipse.cdt.internal.core.model.ASTCache.ASTRunnable; /** * Provides a shared AST for clients. The shared AST is @@ -325,11 +327,22 @@ public final class ASTProvider { fCache.reconciled(ast, (ITranslationUnit) cElement); } + /** + * Executes {@link ASTRunnable#runOnAST(ILanguage, IASTTranslationUnit)} + * with the shared AST for the given translation unit. Handles acquiring + * and releasing the index read-lock for the client. + * + * @param cElement the translation unit + * @param waitFlag condition for waiting for the AST to be built. + * @param monitor a progress monitor, may be null + * @param astRunnable the runnable taking the AST + * @return the status returned by the ASTRunnable + */ public IStatus runOnAST(ICElement cElement, WAIT_FLAG waitFlag, IProgressMonitor monitor, ASTCache.ASTRunnable astRunnable) { Assert.isTrue(cElement instanceof ITranslationUnit); final ITranslationUnit tu = (ITranslationUnit) cElement; - if (!canUseCache(tu, waitFlag)) + if (!prepareForUsingCache(tu, waitFlag)) return Status.CANCEL_STATUS; return fCache.runOnAST(tu, waitFlag != WAIT_NO, monitor, astRunnable); } @@ -352,7 +365,7 @@ public final class ASTProvider { */ public final IASTTranslationUnit acquireSharedAST(ITranslationUnit tu, IIndex index, WAIT_FLAG waitFlag, IProgressMonitor monitor) { - if (!canUseCache(tu, waitFlag)) + if (!prepareForUsingCache(tu, waitFlag)) return null; return fCache.acquireSharedAST(tu, index, waitFlag != WAIT_NO, monitor); } @@ -369,7 +382,15 @@ public final class ASTProvider { fCache.releaseSharedAST(ast); } - private synchronized boolean canUseCache(ITranslationUnit tu, WAIT_FLAG waitFlag) { + /** + * Prepares the AST cache to be used for the given translation unit. + * + * @param tu the translation unit. + * @param waitFlag condition for waiting for the AST to be built. + * @return true if the AST cache can be used for the given translation unit, + * false otherwise. + */ + private boolean prepareForUsingCache(ITranslationUnit tu, WAIT_FLAG waitFlag) { final boolean isActive= fCache.isActiveElement(tu); if (!tu.isOpen()) return false;