diff --git a/api/src/org/labkey/api/data/DbScope.java b/api/src/org/labkey/api/data/DbScope.java index 1dcd1542550..1667e6b66bb 100644 --- a/api/src/org/labkey/api/data/DbScope.java +++ b/api/src/org/labkey/api/data/DbScope.java @@ -2574,6 +2574,16 @@ private void popCurrentTransaction() } } + // Counterpart to popCurrentTransaction(), for callers that need to detach a transaction from its thread + // temporarily. See TransactionImpl.commitAndKeepConnection(). + private void pushCurrentTransaction(TransactionImpl transaction) + { + synchronized (_transaction) + { + _transaction.computeIfAbsent(getEffectiveThread(), _ -> new ArrayList<>()).add(transaction); + } + } + public static class ConnectionSharingCloseable implements AutoCloseable { private final Thread _asyncThread; @@ -2805,8 +2815,29 @@ public void commitAndKeepConnection() { CommitTaskOption.PRECOMMIT.run(this); getConnection().commit(); - _caches.clear(); - CommitTaskOption.POSTCOMMIT.run(this); + closeCaches(); + + // Detach this transaction from the thread while the POSTCOMMIT tasks run, matching commit(), which + // pops before running them. Commit tasks that invalidate a DatabaseCache resolve their target through + // getCurrentTransactionImpl(): with this transaction still on the thread they build a fresh + // TransactionCache and clear that throwaway private cache, so the shared cache goes on serving + // pre-commit values until they expire. Skip the swap if we somehow aren't the innermost transaction, + // since popping would then corrupt the thread's transaction stack. + boolean detached = this == getCurrentTransactionImpl(); + + if (detached) + popCurrentTransaction(); + + try + { + CommitTaskOption.POSTCOMMIT.run(this); + } + finally + { + if (detached) + pushCurrentTransaction(this); + } + clearCommitTasks(); } catch (SQLException e) @@ -3241,6 +3272,59 @@ public void tesCommitTaskFailure() closeAllConnectionsForCurrentThread(); } + @Test + public void testCommitAndKeepConnection() + { + DbScope scope = getLabKeyScope(); + // TempDatabaseCache's shared cache is temporary, so it stays out of KNOWN_CACHES; close() it below + DatabaseCache cache = new DatabaseCache.TestCase.TempDatabaseCache<>(scope, 10, "commitAndKeepConnection test"); + + try + { + cache.put("key_1", "value_1"); + cache.put("key_2", "value_2"); + + List transactionsSeenByPostCommitTask = new ArrayList<>(); + + try (Transaction t = scope.ensureTransaction()) + { + t.addCommitTask(() -> transactionsSeenByPostCommitTask.add(scope.getCurrentTransaction()), CommitTaskOption.POSTCOMMIT); + cache.remove("key_1"); + + // DatabaseCache defers removals to the commit, so the shared cache still serves the old value + assertTrue("Shared cache should still hold key_1 before the commit", cache.getKeys().contains("key_1")); + + t.commitAndKeepConnection(); + + // The deferred removal must land on the shared cache. If this transaction is still on the thread + // while the POSTCOMMIT tasks run, the removal builds a fresh TransactionCache and clears that + // throwaway private cache instead, leaving key_1 in the shared cache until it expires. + assertFalse("commitAndKeepConnection() must invalidate the shared cache", cache.getKeys().contains("key_1")); + assertTrue("commitAndKeepConnection() should leave unrelated keys alone", cache.getKeys().contains("key_2")); + + // POSTCOMMIT tasks must run detached from the transaction, exactly as they do under commit() + assertEquals("POSTCOMMIT task should have run exactly once", 1, transactionsSeenByPostCommitTask.size()); + assertNull("POSTCOMMIT tasks must not see an active transaction", transactionsSeenByPostCommitTask.get(0)); + + // ...and the transaction must be back on the thread, still active and still usable + assertTrue(scope.isTransactionActive()); + assertSame("commitAndKeepConnection() must leave the transaction on the thread", t, scope.getCurrentTransaction()); + + cache.remove("key_2"); + assertTrue("Removal after commitAndKeepConnection() should be deferred again", cache.getKeys().contains("key_2")); + + t.commit(); + } + + assertFalse("commit() must invalidate the shared cache", cache.getKeys().contains("key_2")); + assertFalse(scope.isTransactionActive()); + } + finally + { + cache.close(); + } + } + @Test public void testLockReleasedException() { diff --git a/core/src/org/labkey/core/attachment/AttachmentCache.java b/core/src/org/labkey/core/attachment/AttachmentCache.java index 559c7ea12d2..a791c4b4c5c 100644 --- a/core/src/org/labkey/core/attachment/AttachmentCache.java +++ b/core/src/org/labkey/core/attachment/AttachmentCache.java @@ -25,6 +25,7 @@ import org.labkey.api.collections.CsvSet; import org.labkey.api.data.Container; import org.labkey.api.data.CoreSchema; +import org.labkey.api.data.DatabaseCache; import org.labkey.api.data.SimpleFilter; import org.labkey.api.data.Sort; import org.labkey.api.data.TableSelector; @@ -44,7 +45,6 @@ public class AttachmentCache { private static final Set ATTACHMENT_COLUMNS = new CsvSet("Parent, Container, DocumentName, DocumentSize, DocumentType, Created, CreatedBy, LastIndexed"); - private static final Cache> CACHE = CacheManager.getStringKeyCache(200000, CacheManager.MONTH, "Attachments"); private static final CacheLoader> LOADER = (key, attachmentParent) -> { @@ -63,6 +63,9 @@ public class AttachmentCache return Collections.unmodifiableMap(map); }; + // Must be transaction aware: attachments are very often added and deleted inside a transaction + private static final Cache> CACHE = DatabaseCache.get(CoreSchema.getInstance().getScope(), 200000, CacheManager.MONTH, "Attachments", LOADER); + static @NotNull Map getAttachments(AttachmentParent parent) { @@ -82,6 +85,14 @@ static void removeAttachments(Container c) } + // Wholesale invalidation, for bulk operations where invalidating every affected parent individually would + // retain too many deferred commit tasks. + static void clear() + { + CACHE.clear(); + } + + private static String getKey(AttachmentParent parent) { return parent.getContainerId() + ":" + parent.getEntityId(); diff --git a/core/src/org/labkey/core/attachment/AttachmentServiceImpl.java b/core/src/org/labkey/core/attachment/AttachmentServiceImpl.java index 59f3aa8182e..f96f0d7c33d 100644 --- a/core/src/org/labkey/core/attachment/AttachmentServiceImpl.java +++ b/core/src/org/labkey/core/attachment/AttachmentServiceImpl.java @@ -33,6 +33,7 @@ import org.labkey.api.attachments.AttachmentParent; import org.labkey.api.attachments.AttachmentParentType; import org.labkey.api.attachments.AttachmentService; +import org.labkey.api.attachments.ByteArrayAttachmentFile; import org.labkey.api.attachments.DocumentWriter; import org.labkey.api.attachments.FileAttachmentFile; import org.labkey.api.attachments.SpringAttachmentFile; @@ -91,6 +92,7 @@ import org.labkey.api.util.GUID; import org.labkey.api.util.HtmlString; import org.labkey.api.util.HtmlStringBuilder; +import org.labkey.api.util.JunitUtil; import org.labkey.api.util.MimeMap; import org.labkey.api.util.PageFlowUtil; import org.labkey.api.util.Pair; @@ -146,6 +148,7 @@ import java.util.Objects; import java.util.Set; import java.util.TreeSet; +import java.util.concurrent.atomic.AtomicReference; import java.util.stream.Collectors; public class AttachmentServiceImpl implements AttachmentService @@ -155,6 +158,10 @@ public class AttachmentServiceImpl implements AttachmentService private static final Map ATTACHMENT_TYPE_MAP = new HashMap<>(); private static final Set ATTACHMENT_COLUMNS = Set.of("Parent", "Container", "DocumentName", "DocumentSize", "DocumentType", "Created", "CreatedBy", "LastIndexed"); + // Maximum number of deferred AttachmentCache invalidations one transaction will accumulate before falling + // back to clearing the whole cache. See invalidateCache(). + private static final int MAX_DEFERRED_CACHE_INVALIDATIONS = 1000; + @Override public void download(HttpServletResponse response, AttachmentParent parent, String filename, @Nullable String alias, boolean inlineIfPossible) throws ServletException, IOException { @@ -290,9 +297,13 @@ public synchronized void addAttachments(AttachmentParent parent, List filesToSkip = new TreeSet<>(); File fileLocation = parent instanceof AttachmentDirectory dir ? dir.getFileSystemDirectory() : null; + // Resolve which of these files already exist with a single query instead of a cached read per file. + Map existingAttachments = null == parent ? Collections.emptyMap() : + getAttachments(parent, files.stream().map(AttachmentFile::getFilename).toList()); + for (AttachmentFile file : files) { - if (parent != null && exists(parent, file.getFilename())) + if (existingAttachments.containsKey(file.getFilename())) { filesToSkip.add(file.getFilename()); continue; @@ -318,7 +329,7 @@ public synchronized void addAttachments(AttachmentParent parent, List getAttachmentsForDelete(AttachmentParent parent) + { + if (parent instanceof AttachmentDirectory) + return getAttachments(parent); + + checkSecurityPolicy(parent); + + return new TableSelector(CoreSchema.getInstance().getTableInfoDocuments(), + ATTACHMENT_COLUMNS, + new SimpleFilter(FieldKey.fromParts("Parent"), parent.getEntityId()), + new Sort("+RowId")).getArrayList(Attachment.class); + } + @Override public void deleteAttachments(Collection parents) { for (AttachmentParent parent : parents) { - List atts = getAttachments(parent); + List atts = getAttachmentsForDelete(parent); // No attachments, or perhaps container doesn't match entityid if (atts.isEmpty()) @@ -399,7 +431,89 @@ public void deleteAttachments(Collection parents) new SqlExecutor(coreTables().getSchema()).execute(sqlCascadeDelete(parent)); if (parent instanceof AttachmentDirectory) ((AttachmentDirectory)parent).deleteAttachment(HttpView.currentContext().getUser(), null); + invalidateCache(parent); + } + } + + /** + * Invalidates a mutated parent's AttachmentCache entry, bounding how many deferred invalidations a single + * transaction can accumulate. Inside a transaction, DatabaseCache defers each key removal to the commit as its + * own task (see TransactionCache.remove()), and several callers reach this once per parent inside a single + * transaction: ExperimentServiceImpl.truncateDataClass builds one AttachmentParent per data class row, + * IssueManager one per comment, and AttachmentDataIterator adds and deletes attachments per imported row. A + * large operation would otherwise retain a task per parent for the life of the transaction. Past + * MAX_DEFERRED_CACHE_INVALIDATIONS we clear the whole cache once instead, trading precision for a bound on + * memory; over-invalidating only costs cache warmth, never correctness. + */ + private void invalidateCache(AttachmentParent parent) + { + DbScope.Transaction tx = CoreSchema.getInstance().getScope().getCurrentTransaction(); + + // Outside a transaction each removal happens immediately and retains nothing, so there's nothing to bound + if (null == tx) + { + AttachmentCache.removeAttachments(parent); + return; + } + + DeferredInvalidationCounter counter = tx.addCommitTask(new DeferredInvalidationCounter(), DbScope.CommitTaskOption.POSTCOMMIT); + + if (counter.removeIndividually()) AttachmentCache.removeAttachments(parent); + else if (counter.shouldClear()) + AttachmentCache.clear(); + } + + /** + * Transaction-scoped counter of deferred AttachmentCache removals. Coalesced via addCommitTask()'s equals()-based + * dedup, which hands back the task already registered for this transaction; the same approach + * DatabaseCache.BlockingDatabaseCache.CacheReloadCounterTask uses to bound its own post-commit work. run() is a + * no-op -- the task exists only to carry the count. + */ + private static class DeferredInvalidationCounter implements Runnable + { + private int _count; + private boolean _cleared; + + @Override + public void run() + { + // No-op: this task exists only to hold a transaction-scoped count + } + + /** @return true if the caller should invalidate this parent's key individually */ + private boolean removeIndividually() + { + return !_cleared && ++_count <= MAX_DEFERRED_CACHE_INVALIDATIONS; + } + + /** @return true if the caller should clear the whole cache, i.e., only on the call that crosses the limit */ + private boolean shouldClear() + { + if (_cleared) + return false; + + _cleared = true; + return true; + } + + @Override + public boolean equals(Object o) + { + return null != o && getClass() == o.getClass(); + } + + @Override + public int hashCode() + { + return getClass().hashCode(); + } + + // CommitTaskOption.add() logs this eagerly on every duplicate, so keep it allocation-free + @Override + public String toString() + { + return "AttachmentCache deferred invalidation counter"; } } @@ -481,7 +595,7 @@ public void deleteAttachment(AttachmentParent parent, String name, @Nullable Use if (null != att) { _deleteAttachment(parent, name, auditUser); - AttachmentCache.removeAttachments(parent); + invalidateCache(parent); } } @@ -495,7 +609,7 @@ public void deleteAttachments(AttachmentParent parent, Collection names, _deleteAttachment(parent, attachment.getName(), null); } - AttachmentCache.removeAttachments(parent); + invalidateCache(parent); } @@ -530,7 +644,7 @@ public void renameAttachment(AttachmentParent parent, String oldName, String new if (null != dir) src.renameTo(dest); - AttachmentCache.removeAttachments(parent); + invalidateCache(parent); addAuditEvent(auditUser, parent, newName, "The attachment " + oldName + " was renamed " + newName); } @@ -577,7 +691,7 @@ public int moveAttachments(Container newContainer, List parent deleteIndexedAttachment(parent, filename); addAuditEvent(auditUser, parent, filename, "The attachment " + filename + " was moved"); } - AttachmentCache.removeAttachments(parent); + invalidateCache(parent); } } @@ -1832,6 +1946,174 @@ private void testFileAttachmentFiles(File file1, File file2, User user) throws I assertEquals(originalCount, attachments.size()); } + /** + * Attachments are routinely added and deleted inside a transaction while other threads -- most notably the + * SearchService indexing threads -- read the same parent's attachments. Those readers have no transaction, so + * they must see the pre-commit state until the delete commits, and must never be handed a cached snapshot that + * outlives the commit. This is the deterministic version of the race that made + * DeleteJobAttachmentsApiTest.testMultipleAttachments flaky. + */ + @Test + public void testCacheConsistencyAcrossUncommittedDelete() throws Exception + { + User user = TestContext.get().getUser(); + AttachmentService svc = AttachmentService.get(); + AttachmentParent parent = new TestAttachmentParent(JunitUtil.getTestContainer()); + + try + { + svc.addAttachments(parent, List.of(testFile("one.txt"), testFile("two.txt"), testFile("three.txt")), user); + + // Warm the shared cache; this is what threads without a transaction read below + assertEquals("Should start with three attachments", 3, svc.getAttachments(parent).size()); + + try (DbScope.Transaction tx = CoreSchema.getInstance().getScope().ensureTransaction()) + { + svc.deleteAttachment(parent, "one.txt", user); + svc.deleteAttachment(parent, "two.txt", user); + + // The deleting thread sees its own uncommitted deletes + assertEquals(List.of("three.txt"), getNames(svc.getAttachments(parent))); + + // A thread with no transaction must still see the pre-commit state. Two ways this can break: the + // cache hands it the uncommitted list the line above just loaded, or the cache was evicted + // eagerly and it reloads the pre-delete rows on its own connection and caches them past the + // commit. A transaction-aware cache keeps the transaction's loads private and defers the eviction. + assertEquals("Thread without a transaction must see the pre-commit state", + List.of("one.txt", "two.txt", "three.txt"), getNamesOnNewThread(parent)); + + tx.commit(); + } + + // The commit must have invalidated the shared cache in both directions + assertEquals(List.of("three.txt"), getNames(svc.getAttachments(parent))); + assertEquals("Thread without a transaction must see the committed state", + List.of("three.txt"), getNamesOnNewThread(parent)); + } + finally + { + svc.deleteAttachments(parent); + } + } + + /** + * The bulk delete path bounds how many deferred cache removals one transaction accumulates (see + * invalidateCache()). Below that bound each parent is still invalidated individually, so verify that + * a multi-parent delete defers its removals and invalidates every parent on commit. + */ + @Test + public void testBulkDeleteCacheInvalidation() throws Exception + { + User user = TestContext.get().getUser(); + AttachmentService svc = AttachmentService.get(); + Container c = JunitUtil.getTestContainer(); + List parents = List.of(new TestAttachmentParent(c), new TestAttachmentParent(c), new TestAttachmentParent(c)); + + try + { + for (AttachmentParent parent : parents) + svc.addAttachments(parent, List.of(testFile("bulk.txt")), user); + + // Warm the shared cache for every parent; this is what threads without a transaction read below + for (AttachmentParent parent : parents) + assertEquals(List.of("bulk.txt"), getNames(svc.getAttachments(parent))); + + try (DbScope.Transaction tx = CoreSchema.getInstance().getScope().ensureTransaction()) + { + svc.deleteAttachments(parents); + + // The deleting thread sees its own uncommitted deletes + for (AttachmentParent parent : parents) + assertTrue("Deleting thread should see its own uncommitted deletes", svc.getAttachments(parent).isEmpty()); + + // ...while the removals stay deferred for everyone else + for (AttachmentParent parent : parents) + assertEquals("Thread without a transaction must see the pre-commit state", + List.of("bulk.txt"), getNamesOnNewThread(parent)); + + tx.commit(); + } + + // Every parent's entry must be gone from the shared cache, not just the last one + for (AttachmentParent parent : parents) + { + assertTrue("Bulk delete must invalidate the shared cache", svc.getAttachments(parent).isEmpty()); + assertEquals(List.of(), getNamesOnNewThread(parent)); + } + } + finally + { + svc.deleteAttachments(parents); + } + } + + private static AttachmentFile testFile(String name) + { + return new ByteArrayAttachmentFile(name, name.getBytes(StringUtilsLabKey.DEFAULT_CHARSET), "text/plain"); + } + + private static List getNames(Collection attachments) + { + return attachments.stream().map(Attachment::getName).toList(); + } + + // Reads the parent's attachments on a thread that has never had a transaction, so it always resolves to the + // shared cache + private static List getNamesOnNewThread(AttachmentParent parent) throws InterruptedException + { + AtomicReference> names = new AtomicReference<>(); + AtomicReference failure = new AtomicReference<>(); + + Thread thread = new Thread(() -> { + try + { + names.set(getNames(AttachmentService.get().getAttachments(parent))); + } + catch (Throwable t) + { + failure.set(t); + } + }, "AttachmentServiceImpl.TestCase reader"); + + thread.start(); + thread.join(30_000); + + if (null != failure.get()) + throw new RuntimeException("Reader thread failed", failure.get()); + assertFalse("Reader thread did not finish; it is likely blocked on the open transaction", thread.isAlive()); + + return names.get(); + } + + private static class TestAttachmentParent implements AttachmentParent + { + private final String _containerId; + private final String _entityId = GUID.makeGUID(); + + private TestAttachmentParent(Container c) + { + _containerId = c.getId(); + } + + @Override + public String getEntityId() + { + return _entityId; + } + + @Override + public String getContainerId() + { + return _containerId; + } + + @Override + public @NotNull AttachmentParentType getAttachmentParentType() + { + return AttachmentParentType.UNKNOWN; + } + } + // Tests the ability to extract EntityIds from data class LSIDs @Test public void testLsidGuidExtraction()