From 571d62d6f970c3de08a98d0d03f06d934240132f Mon Sep 17 00:00:00 2001 From: Jonah Graham Date: Mon, 7 Nov 2022 11:40:14 -0500 Subject: [PATCH] Stop trying to clear result caches when lock isn't held The indexer has a feature that allows readers of the index to read the index in the middle of write operations. This is done by using a YeildableIndexLock. The YeildableIndexLock's yield method can be called to temporarily give up the write lock. However the assumption in the code was that it would always successfully reaquire the lock after that. However, if the indexing was cancelled the lock would fail to be reaquired. Therefore the code that thinks it owns the lock no longer owns it. In this case the code in PDOMWriter.storeSymbolsInIndex's finally block. Therefore I have added an new exception type to explicitly identify this use case so the original code can differentiate between cases where an exception was thrown where the lock is still held, and cases where the lock is no longer held. Note that instead of a new exception caught like this: ```java } catch (FailedToReAcquireLockException e) { hasLock = false; e.reThrow(); ``` I could have done this: ```java } catch (InterruptedException | OperationCanceledException e) { hasLock = false; throw e; ``` But it is not obvious that nothing else other than the acquire can raise an OperationCanceledException because it is a RuntimeException. By having a new checked exception we can know for sure that in the finally block we have lost our lock. There are no API implications of this change as all the classes and interfaces are internal to CDT. Fixes #128 --- .../internal/core/index/IWritableIndex.java | 3 +- .../core/index/IWritableIndexFragment.java | 3 +- .../internal/core/index/WritableCIndex.java | 3 +- .../pdom/FailedToReAcquireLockException.java | 42 +++++++++++++++++++ .../cdt/internal/core/pdom/PDOMWriter.java | 24 ++++++++--- .../cdt/internal/core/pdom/WritablePDOM.java | 2 +- .../core/pdom/YieldableIndexLock.java | 16 ++++--- .../cdt/internal/core/pdom/dom/PDOMFile.java | 4 +- 8 files changed, 81 insertions(+), 16 deletions(-) create mode 100644 core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/FailedToReAcquireLockException.java diff --git a/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/index/IWritableIndex.java b/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/index/IWritableIndex.java index de6e7f51434..61c5c6776b0 100644 --- a/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/index/IWritableIndex.java +++ b/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/index/IWritableIndex.java @@ -22,6 +22,7 @@ import org.eclipse.cdt.core.index.IIndex; import org.eclipse.cdt.core.index.IIndexFileLocation; import org.eclipse.cdt.core.parser.ISignificantMacros; import org.eclipse.cdt.internal.core.pdom.ASTFilePathResolver; +import org.eclipse.cdt.internal.core.pdom.FailedToReAcquireLockException; import org.eclipse.cdt.internal.core.pdom.YieldableIndexLock; import org.eclipse.core.runtime.CoreException; import org.eclipse.core.runtime.IProgressMonitor; @@ -116,7 +117,7 @@ public interface IWritableIndex extends IIndex { */ void setFileContent(IIndexFragmentFile sourceFile, int linkageID, IncludeInformation[] includes, IASTPreprocessorStatement[] macros, IASTName[][] names, ASTFilePathResolver resolver, - YieldableIndexLock lock) throws CoreException, InterruptedException; + YieldableIndexLock lock) throws CoreException, FailedToReAcquireLockException; /** * Clears the entire index. diff --git a/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/index/IWritableIndexFragment.java b/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/index/IWritableIndexFragment.java index 01652646e90..fec56a17747 100644 --- a/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/index/IWritableIndexFragment.java +++ b/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/index/IWritableIndexFragment.java @@ -21,6 +21,7 @@ import org.eclipse.cdt.core.index.IIndexFileLocation; import org.eclipse.cdt.core.parser.ISignificantMacros; import org.eclipse.cdt.internal.core.index.IWritableIndex.IncludeInformation; import org.eclipse.cdt.internal.core.pdom.ASTFilePathResolver; +import org.eclipse.cdt.internal.core.pdom.FailedToReAcquireLockException; import org.eclipse.cdt.internal.core.pdom.YieldableIndexLock; import org.eclipse.core.runtime.CoreException; import org.eclipse.core.runtime.IProgressMonitor; @@ -84,7 +85,7 @@ public interface IWritableIndexFragment extends IIndexFragment { */ void addFileContent(IIndexFragmentFile sourceFile, IncludeInformation[] includes, IASTPreprocessorStatement[] macros, IASTName[][] names, ASTFilePathResolver resolver, - YieldableIndexLock lock) throws CoreException, InterruptedException; + YieldableIndexLock lock) throws CoreException, FailedToReAcquireLockException; /** * Acquires a write lock, while giving up a certain amount of read locks. diff --git a/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/index/WritableCIndex.java b/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/index/WritableCIndex.java index 043d4f025dc..4db1d8e45ed 100644 --- a/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/index/WritableCIndex.java +++ b/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/index/WritableCIndex.java @@ -20,6 +20,7 @@ import org.eclipse.cdt.core.dom.ast.IASTPreprocessorStatement; import org.eclipse.cdt.core.index.IIndexFileLocation; import org.eclipse.cdt.core.parser.ISignificantMacros; import org.eclipse.cdt.internal.core.pdom.ASTFilePathResolver; +import org.eclipse.cdt.internal.core.pdom.FailedToReAcquireLockException; import org.eclipse.cdt.internal.core.pdom.YieldableIndexLock; import org.eclipse.core.runtime.CoreException; import org.eclipse.core.runtime.IProgressMonitor; @@ -78,7 +79,7 @@ public class WritableCIndex extends CIndex implements IWritableIndex { @Override public void setFileContent(IIndexFragmentFile file, int linkageID, IncludeInformation[] includes, IASTPreprocessorStatement[] macros, IASTName[][] names, ASTFilePathResolver resolver, - YieldableIndexLock lock) throws CoreException, InterruptedException { + YieldableIndexLock lock) throws CoreException, FailedToReAcquireLockException { assert getWritableFragment() == file.getIndexFragment(); for (IncludeInformation include : includes) { diff --git a/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/FailedToReAcquireLockException.java b/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/FailedToReAcquireLockException.java new file mode 100644 index 00000000000..4ffc1ccdeb8 --- /dev/null +++ b/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/FailedToReAcquireLockException.java @@ -0,0 +1,42 @@ +/******************************************************************************* + * Copyright (c) 2022 Kichwa Coders Canada, Inc. and others. + * + * This program and the accompanying materials + * are made available under the terms of the Eclipse Public License 2.0 + * which accompanies this distribution, and is available at + * https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + *******************************************************************************/ +package org.eclipse.cdt.internal.core.pdom; + +import org.eclipse.core.runtime.Assert; +import org.eclipse.core.runtime.OperationCanceledException; + +/** + * This exception is raised when {@link YieldableIndexLock#yield()} fails to + * reacquire the lock after yielding, this may be due to an {@link InterruptedException} + * or an {@link OperationCanceledException} which will be the nested exception. + */ +public class FailedToReAcquireLockException extends Exception { + + public FailedToReAcquireLockException(InterruptedException e) { + super(e); + Assert.isNotNull(e); + } + + public FailedToReAcquireLockException(OperationCanceledException e) { + super(e); + Assert.isNotNull(e); + } + + public void reThrow() throws InterruptedException, OperationCanceledException { + if (getCause() instanceof InterruptedException ie) { + throw ie; + } + if (getCause() instanceof OperationCanceledException oce) { + throw oce; + } + throw new RuntimeException("Unexpectedly the exception cause was none of the allowed types", this); //$NON-NLS-1$ + } +} diff --git a/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/PDOMWriter.java b/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/PDOMWriter.java index 566cb463d36..6f26581a1cd 100644 --- a/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/PDOMWriter.java +++ b/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/PDOMWriter.java @@ -322,6 +322,7 @@ public abstract class PDOMWriter implements IPDOMASTProcessor { Throwable th = null; YieldableIndexLock lock = new YieldableIndexLock(data.fIndex, false, progress.split(1)); lock.acquire(); + boolean hasLock = true; try { final boolean isReplacement = ctx != null && fileInAST.includeStatement == null; IIndexFragmentFile ifile = null; @@ -342,17 +343,28 @@ public abstract class PDOMWriter implements IPDOMASTProcessor { } } } + } catch (FailedToReAcquireLockException e) { + hasLock = false; + e.reThrow(); } catch (OperationCanceledException e) { throw e; } catch (RuntimeException | StackOverflowError | AssertionError e) { th = e; } finally { - // Because the caller holds a read-lock, the result cache of the index is never cleared. - // Before releasing the lock for the last time in this AST, we clear the result cache. - if (i == data.fSelectedFiles.length - 1) { - data.fIndex.clearResultCache(); + // If we failed to reacquire the yieldable index lock after yielding (for example + // because the project got closed) then FailedToReAcquireLockException will + // have been thrown and we are done here. + // + // Note - we can't ask the index if it is locked because someone else may have now + // locked it once we failed to reacquire the lock. + if (hasLock) { + // Because the caller holds a read-lock, the result cache of the index is never cleared. + // Before releasing the lock for the last time in this AST, we clear the result cache. + if (i == data.fSelectedFiles.length - 1) { + data.fIndex.clearResultCache(); + } + lock.release(); } - lock.release(); } if (th != null) { data.fStatuses.add(createStatus(NLS.bind(Messages.PDOMWriter_errorWhileParsing, @@ -619,7 +631,7 @@ public abstract class PDOMWriter implements IPDOMASTProcessor { } private IIndexFragmentFile storeFileInIndex(Data data, FileInAST astFile, int storageLinkageID, - YieldableIndexLock lock, IProgressMonitor monitor) throws CoreException, InterruptedException { + YieldableIndexLock lock, IProgressMonitor monitor) throws CoreException, FailedToReAcquireLockException { final IWritableIndex index = data.fIndex; IIndexFragmentFile file; // We create a temporary PDOMFile with zero timestamp, add names to it, then replace diff --git a/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/WritablePDOM.java b/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/WritablePDOM.java index fd38a885dd9..5a15ca45b26 100644 --- a/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/WritablePDOM.java +++ b/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/WritablePDOM.java @@ -147,7 +147,7 @@ public class WritablePDOM extends PDOM implements IWritableIndexFragment { @Override public void addFileContent(IIndexFragmentFile sourceFile, IncludeInformation[] includes, IASTPreprocessorStatement[] macros, IASTName[][] names, ASTFilePathResolver pathResolver, - YieldableIndexLock lock) throws CoreException, InterruptedException { + YieldableIndexLock lock) throws CoreException, FailedToReAcquireLockException { assert sourceFile.getIndexFragment() == this; PDOMFile pdomFile = (PDOMFile) sourceFile; diff --git a/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/YieldableIndexLock.java b/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/YieldableIndexLock.java index b15f1f91232..618ffd175c2 100644 --- a/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/YieldableIndexLock.java +++ b/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/YieldableIndexLock.java @@ -15,6 +15,7 @@ package org.eclipse.cdt.internal.core.pdom; import org.eclipse.cdt.internal.core.index.IWritableIndex; import org.eclipse.core.runtime.IProgressMonitor; +import org.eclipse.core.runtime.OperationCanceledException; /** * Write lock on the index that can be yielded temporarily to unblock threads that need @@ -56,16 +57,21 @@ public class YieldableIndexLock { } /** - * Yields the lock temporarily if it was held for YIELD_INTERVAL or more, and somebody is waiting - * for a read lock. - * @throws InterruptedException + * Yields the lock temporarily if somebody is waiting for a read lock. + * @throws FailedToReAcquireLockException when lock is not reacquired. */ - public void yield() throws InterruptedException { + public void yield() throws FailedToReAcquireLockException { if (index.hasWaitingReaders()) { index.releaseWriteLock(false); cumulativeLockTime += System.currentTimeMillis() - lastLockTime; lastLockTime = 0; - acquire(); + try { + acquire(); + } catch (OperationCanceledException e) { + throw new FailedToReAcquireLockException(e); + } catch (InterruptedException e) { + throw new FailedToReAcquireLockException(e); + } } } diff --git a/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/PDOMFile.java b/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/PDOMFile.java index fa841dd5ace..ce0e94ba499 100644 --- a/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/PDOMFile.java +++ b/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/PDOMFile.java @@ -47,6 +47,7 @@ import org.eclipse.cdt.internal.core.index.IWritableIndex.IncludeInformation; import org.eclipse.cdt.internal.core.index.IWritableIndexFragment; import org.eclipse.cdt.internal.core.index.IndexFileLocation; import org.eclipse.cdt.internal.core.parser.scanner.SignificantMacros; +import org.eclipse.cdt.internal.core.pdom.FailedToReAcquireLockException; import org.eclipse.cdt.internal.core.pdom.PDOM; import org.eclipse.cdt.internal.core.pdom.YieldableIndexLock; import org.eclipse.cdt.internal.core.pdom.db.BTree; @@ -472,7 +473,8 @@ public class PDOMFile implements IIndexFragmentFile { return fLinkage; } - public void addNames(IASTName[][] names, YieldableIndexLock lock) throws CoreException, InterruptedException { + public void addNames(IASTName[][] names, YieldableIndexLock lock) + throws CoreException, FailedToReAcquireLockException { assert getFirstName() == null; assert getFirstMacroReference() == null; final PDOMLinkage linkage = getLinkage();