diff --git a/inception/inception-annotation-storage/src/main/java/de/tudarmstadt/ukp/inception/annotation/storage/CasStorageServiceImpl.java b/inception/inception-annotation-storage/src/main/java/de/tudarmstadt/ukp/inception/annotation/storage/CasStorageServiceImpl.java index 6bfa745f51d..20828d6d8d1 100644 --- a/inception/inception-annotation-storage/src/main/java/de/tudarmstadt/ukp/inception/annotation/storage/CasStorageServiceImpl.java +++ b/inception/inception-annotation-storage/src/main/java/de/tudarmstadt/ukp/inception/annotation/storage/CasStorageServiceImpl.java @@ -1012,15 +1012,12 @@ public void exportCas(SourceDocument aDocument, AnnotationSet aSet, OutputStream { // Ensure that the CAS is not being re-written and temporarily unavailable while we export // it, then add this info to a mini-session to ensure that write-access is known - try (var session = CasStorageSession.openNested(true)) { + try (var session = CasStorageSession.openNested()) { try (var access = new WithExclusiveAccess(aDocument, aSet)) { - session.add(aDocument.getId(), aSet, EXCLUSIVE_WRITE_ACCESS, access.getHolder()); + access.ensureRegisteredInSession(); driver.exportCas(aDocument, aSet, aStream); } - finally { - session.remove(aDocument.getId(), aSet); - } } catch (IOException e) { throw e; @@ -1034,17 +1031,14 @@ public void exportCas(SourceDocument aDocument, AnnotationSet aSet, OutputStream public void importCas(SourceDocument aDocument, AnnotationSet aSet, InputStream aStream) throws IOException { - // Ensure that the CAS is not being re-written and temporarily unavailable while we export + // Ensure that the CAS is not being re-written and temporarily unavailable while we import // it, then add this info to a mini-session to ensure that write-access is known - try (var session = CasStorageSession.openNested(true)) { + try (var session = CasStorageSession.openNested()) { try (var access = new WithExclusiveAccess(aDocument, aSet)) { - session.add(aDocument.getId(), aSet, EXCLUSIVE_WRITE_ACCESS, access.getHolder()); + access.ensureRegisteredInSession(); driver.importCas(aDocument, aSet, aStream); } - finally { - session.remove(aDocument.getId(), aSet); - } } catch (IOException e) { throw e; @@ -1073,9 +1067,9 @@ public void forceActionOnCas(SourceDocument aDocument, AnnotationSet aSet, { // Ensure that the CAS is not being re-written and temporarily unavailable while we check // upgrade it, then add this info to a mini-session to ensure that write-access is known - try (var session = CasStorageSession.openNested(true)) { + try (var session = CasStorageSession.openNested()) { try (var access = new WithExclusiveAccess(aDocument, aSet)) { - session.add(aDocument.getId(), aSet, EXCLUSIVE_WRITE_ACCESS, access.getHolder()); + access.ensureRegisteredInSession(); var cas = aLoader.load(aDocument, aSet); access.setCas(cas); @@ -1086,9 +1080,6 @@ public void forceActionOnCas(SourceDocument aDocument, AnnotationSet aSet, realWriteCas(aDocument, aSet, cas); } } - finally { - session.remove(aDocument.getId(), aSet); - } } catch (IOException e) { throw e; @@ -1146,6 +1137,12 @@ private class WithExclusiveAccess private long documentId; private AnnotationSet set; + /** The session this access lives in. */ + private final CasStorageSession session; + + /** Whether this access has been registered in the {@link #session}. */ + private boolean registered; + public WithExclusiveAccess(SourceDocument aDocument, AnnotationSet aSet) throws CasSessionException { @@ -1154,7 +1151,7 @@ public WithExclusiveAccess(SourceDocument aDocument, AnnotationSet aSet) documentId = aDocument.getId(); set = aSet; - var session = CasStorageSession.get(); + session = CasStorageSession.get(); if (!session.hasExclusiveAccess(aDocument, aSet)) { LOG.trace("CAS storage session [{}]: trying to briefly borrow CAS [{}]@{}", @@ -1179,6 +1176,33 @@ public CasKey getKey() return key; } + /** + * Registers the exclusive access held by this context in the CAS storage session so that + * operations nested inside the exclusive access scope recognize that write access has + * legitimately been obtained. + *

+ * Most users of {@link WithExclusiveAccess} do not need this: holding the lock is enough to + * keep the CAS from being re-written while e.g. its metadata is inspected. It is required + * only when the locked scope hands the CAS - or the data backing it - to code that may + * itself try to write. + *

+ * Does nothing if an enclosing session already holds exclusive access. In that case the + * access is that session's to register and to release, and this context merely operates + * under it - the CAS is locked either way. The registration is removed again when this + * context is {@link #close() closed}. + *

+ * Calling this method multiple times has no additional effect. + */ + public void ensureRegisteredInSession() + { + if (holder == null || registered) { + return; + } + + session.add(documentId, set, EXCLUSIVE_WRITE_ACCESS, holder); + registered = true; + } + public boolean isCasSet() { if (holder != null) { @@ -1254,6 +1278,11 @@ public void release() @Override public void close() { + if (registered) { + session.remove(documentId, set); + registered = false; + } + if (holder != null) { LOG.trace("Returning briefly borrowed CAS [{}]@[{}]({})", set, documentName, documentId); diff --git a/inception/inception-annotation-storage/src/test/java/de/tudarmstadt/ukp/inception/annotation/storage/CasStorageServiceImplTest.java b/inception/inception-annotation-storage/src/test/java/de/tudarmstadt/ukp/inception/annotation/storage/CasStorageServiceImplTest.java index 5cf8d01f527..2e42b20272e 100644 --- a/inception/inception-annotation-storage/src/test/java/de/tudarmstadt/ukp/inception/annotation/storage/CasStorageServiceImplTest.java +++ b/inception/inception-annotation-storage/src/test/java/de/tudarmstadt/ukp/inception/annotation/storage/CasStorageServiceImplTest.java @@ -41,6 +41,8 @@ import static org.assertj.core.api.Assertions.assertThatExceptionOfType; import static org.junit.jupiter.api.parallel.ExecutionMode.CONCURRENT; +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; import java.io.File; import java.io.FileNotFoundException; import java.io.IOException; @@ -688,6 +690,116 @@ public void returnObject(CasKey aKey, CasHolder aObj) } } + @Test + public void testExportCasWorksWhileEnclosingSessionHoldsExclusiveAccess() throws Exception + { + // A caller which already holds exclusive access to a CAS - e.g. the project exporter after + // lazily creating a missing initial CAS - must still be able to export it. Since the + // exclusive access pool is keyed per document, a second borrow on the same thread can never + // be satisfied: it would block until the borrow timeout and then fail. The export must + // therefore recognize the exclusive access held by the enclosing session instead of trying + // to borrow the CAS again. + var shortTimeoutSut = createStorageService(Duration.ofMillis(250)); + var doc = makeSourceDocument(20l, 20l, "test"); + var set = AnnotationSet.forTest("test"); + + try (var session = openNested(true)) { + createCasFile(shortTimeoutSut, doc, set, "This is a test"); + } + + var buffer = new ByteArrayOutputStream(); + try (var session = CasStorageSession.open()) { + // Take exclusive access and keep it for the duration of the export + shortTimeoutSut.readCas(doc, set, EXCLUSIVE_WRITE_ACCESS); + + shortTimeoutSut.exportCas(doc, set, buffer); + } + + assertThat(buffer.size()).isGreaterThan(0); + } + + @Test + public void testExportCasTakesExclusiveAccessWhenNobodyElseHoldsIt() throws Exception + { + // The counterpart to the test above: when no enclosing session holds the CAS, the export + // must acquire the exclusive access itself so that the CAS cannot be re-written on disk + // while its bytes are being copied. + var doc = makeSourceDocument(21l, 21l, "test"); + var set = AnnotationSet.forTest("test"); + + try (var session = openNested(true)) { + createCasFile(doc, set, "This is a test"); + } + + var buffer = new ByteArrayOutputStream(); + try (var session = CasStorageSession.open()) { + sut.exportCas(doc, set, buffer); + + // The export must not leave the CAS registered in the caller's session + assertThat(session.getManagedState(doc.getId(), set)).isEmpty(); + } + + assertThat(buffer.size()).isGreaterThan(0); + + // The CAS must have been returned to the pool - if it had not, this would block until the + // borrow timeout and then fail + try (var session = CasStorageSession.open()) { + assertThat(sut.readCas(doc, set, EXCLUSIVE_WRITE_ACCESS)).isNotNull(); + } + } + + @Test + public void testImportCasWorksWhileEnclosingSessionHoldsExclusiveAccess() throws Exception + { + // Same as for the export: a caller which already holds exclusive access must be able to + // import into the CAS without the import trying to borrow it a second time. + var shortTimeoutSut = createStorageService(Duration.ofMillis(250)); + var doc = makeSourceDocument(22l, 22l, "test"); + var set = AnnotationSet.forTest("test"); + + var exported = new ByteArrayOutputStream(); + try (var session = CasStorageSession.open()) { + createCasFile(shortTimeoutSut, doc, set, "This is a test"); + shortTimeoutSut.exportCas(doc, set, exported); + } + + try (var session = CasStorageSession.open()) { + // Take exclusive access and keep it for the duration of the import + shortTimeoutSut.readCas(doc, set, EXCLUSIVE_WRITE_ACCESS); + + shortTimeoutSut.importCas(doc, set, new ByteArrayInputStream(exported.toByteArray())); + + assertThat(shortTimeoutSut.existsCas(doc, set)).isTrue(); + } + } + + @Test + public void testForceActionOnCasWorksWhenNobodyElseHoldsExclusiveAccess() throws Exception + { + // Unlike the export, forceActionOnCas replaces the CAS in the exclusive access holder. That + // requires it to have borrowed the CAS itself - it cannot replace a CAS which an enclosing + // session registered - so it is only exercised without an enclosing lock here. + var doc = makeSourceDocument(23l, 23l, "test"); + var set = AnnotationSet.forTest("test"); + + try (var session = openNested(true)) { + createCasFile(doc, set, "This is a test"); + } + + var actionWasApplied = new AtomicBoolean(false); + try (var session = CasStorageSession.open()) { + sut.forceActionOnCas(doc, set, // + (d, s) -> driver.readCas(d, s), // + (d, s, cas) -> actionWasApplied.set(true), // + true); + + // The action must not leave the CAS registered in the caller's session + assertThat(session.getManagedState(doc.getId(), set)).isEmpty(); + } + + assertThat(actionWasApplied).isTrue(); + } + @Test public void testOutOfMemoryDuringExclusiveCasLoadDoesNotStrandCasKey() throws Exception {