From 7b62d51081b0bcd76e84344d7edc641d3c857742 Mon Sep 17 00:00:00 2001 From: DBotThePony Date: Fri, 15 May 2026 11:48:10 +0700 Subject: [PATCH 1/5] Implement SnapshotJournal#hasOngoingTransaction and tests for it --- .../transfer/transaction/SnapshotJournal.java | 36 ++++- .../unittest/transfer/TransactionTests.java | 148 ++++++++++++++++++ 2 files changed, 181 insertions(+), 3 deletions(-) diff --git a/src/main/java/net/neoforged/neoforge/transfer/transaction/SnapshotJournal.java b/src/main/java/net/neoforged/neoforge/transfer/transaction/SnapshotJournal.java index 546cae9992..a066edb53e 100644 --- a/src/main/java/net/neoforged/neoforge/transfer/transaction/SnapshotJournal.java +++ b/src/main/java/net/neoforged/neoforge/transfer/transaction/SnapshotJournal.java @@ -46,6 +46,8 @@ public abstract class SnapshotJournal { private final ArrayList snapshots = new ArrayList<>(); + private boolean hasOngoingTransaction = false; + @Nullable private T originalState = null; @@ -99,6 +101,15 @@ public abstract class SnapshotJournal { */ protected void onRootCommit(T originalState) {} + /** + * Signifies whenever this journal is part of any ongoing transaction. + * + * @return whenever this journal is part of any ongoing transaction + */ + public final boolean hasOngoingTransaction() { + return hasOngoingTransaction; + } + /** * Update the stored snapshots so that the changes happening as part of the passed transaction can be correctly * committed or rolled back. @@ -115,12 +126,14 @@ public abstract class SnapshotJournal { } if (snapshots.get(currentDepth) == NO_SNAPSHOT) { - snapshots.set(currentDepth, createSnapshot()); - // This is a special case where we need to cast to access internal Transaction methods. // You should never, however, cast to call commit or close! var transactionImpl = (Transaction) transaction; transactionImpl.validateOpen(); + + hasOngoingTransaction = true; + snapshots.set(currentDepth, createSnapshot()); + transactionImpl.journalsToClose.add(this); } } @@ -138,7 +151,7 @@ public abstract class SnapshotJournal { // If the transaction was aborted, we just revert to the state of the snapshot. revertToSnapshot(snapshot); releaseSnapshot(snapshot); - } else if (currentDepth <= 0) { + } else if (currentDepth == 0) { // The transaction is the root. if (originalState == null) { originalState = snapshot; @@ -158,6 +171,23 @@ public abstract class SnapshotJournal { // There is already an older snapshot at the depth above, just release the newer one. releaseSnapshot(snapshot); } + + // perform callbacks before declaring this snapshot being no longer part of ongoing transaction + if (currentDepth == 0) { + // root + hasOngoingTransaction = false; + } else { + // snapshot list may have gaps, so we must walk upwards until we hit valid snapshot + for (int i = currentDepth - 1; i >= 0; i--) { + if (snapshots.get(i) != NO_SNAPSHOT) { + hasOngoingTransaction = true; + return; + } + } + + // no snapshot state, we don't belong to a transaction + hasOngoingTransaction = false; + } } void callOnRootCommit() { diff --git a/tests/src/junit/java/net/neoforged/neoforge/unittest/transfer/TransactionTests.java b/tests/src/junit/java/net/neoforged/neoforge/unittest/transfer/TransactionTests.java index 996cfa2b9f..fae08627e5 100644 --- a/tests/src/junit/java/net/neoforged/neoforge/unittest/transfer/TransactionTests.java +++ b/tests/src/junit/java/net/neoforged/neoforge/unittest/transfer/TransactionTests.java @@ -137,6 +137,154 @@ public class TransactionTests { } } + protected static class OngoingTransactionJournal extends SnapshotJournal { + @Override + protected Void createSnapshot() { + assertThat(hasOngoingTransaction()).isEqualTo(true); + return null; + } + + @Override + protected void revertToSnapshot(Void snapshot) { + assertThat(hasOngoingTransaction()).isEqualTo(true); + } + + @Override + protected void onRootCommit(Void originalState) { + assertThat(hasOngoingTransaction()).isEqualTo(false); + } + } + + @Test + void testHasOngoingTransaction() { + var journal = new OngoingTransactionJournal(); + + assertThat(journal.hasOngoingTransaction()).isEqualTo(false); + + // taking part of all transactions + try (var tx = Transaction.openRoot()) { + journal.updateSnapshots(tx); + assertThat(journal.hasOngoingTransaction()).isEqualTo(true); + + try (var tx2 = Transaction.open(tx)) { + journal.updateSnapshots(tx2); + assertThat(journal.hasOngoingTransaction()).isEqualTo(true); + } + + assertThat(journal.hasOngoingTransaction()).isEqualTo(true); + tx.commit(); + assertThat(journal.hasOngoingTransaction()).isEqualTo(false); + } + + assertThat(journal.hasOngoingTransaction()).isEqualTo(false); + + // taking part of root and somewhere downstream + try (var tx = Transaction.openRoot()) { + journal.updateSnapshots(tx); + assertThat(journal.hasOngoingTransaction()).isEqualTo(true); + + try (var tx2 = Transaction.open(tx)) { + try (var tx3 = Transaction.open(tx2)) { + journal.updateSnapshots(tx3); + assertThat(journal.hasOngoingTransaction()).isEqualTo(true); + } + + assertThat(journal.hasOngoingTransaction()).isEqualTo(true); + } + + assertThat(journal.hasOngoingTransaction()).isEqualTo(true); + tx.commit(); + assertThat(journal.hasOngoingTransaction()).isEqualTo(false); + } + + assertThat(journal.hasOngoingTransaction()).isEqualTo(false); + + // taking part only somewhere in downstream, with downstream getting rolled back + try (var tx = Transaction.openRoot()) { + assertThat(journal.hasOngoingTransaction()).isEqualTo(false); + + try (var tx2 = Transaction.open(tx)) { + try (var tx3 = Transaction.open(tx2)) { + journal.updateSnapshots(tx3); + assertThat(journal.hasOngoingTransaction()).isEqualTo(true); + } + + assertThat(journal.hasOngoingTransaction()).isEqualTo(false); + } + + assertThat(journal.hasOngoingTransaction()).isEqualTo(false); + tx.commit(); + assertThat(journal.hasOngoingTransaction()).isEqualTo(false); + } + + assertThat(journal.hasOngoingTransaction()).isEqualTo(false); + + // taking part only somewhere in downstream, with downstream getting commited, then rolled back by parent + try (var tx = Transaction.openRoot()) { + assertThat(journal.hasOngoingTransaction()).isEqualTo(false); + + try (var tx2 = Transaction.open(tx)) { + try (var tx3 = Transaction.open(tx2)) { + journal.updateSnapshots(tx3); + assertThat(journal.hasOngoingTransaction()).isEqualTo(true); + tx3.commit(); + } + + assertThat(journal.hasOngoingTransaction()).isEqualTo(true); + } + + assertThat(journal.hasOngoingTransaction()).isEqualTo(false); + tx.commit(); + assertThat(journal.hasOngoingTransaction()).isEqualTo(false); + } + + assertThat(journal.hasOngoingTransaction()).isEqualTo(false); + + // taking part only somewhere in downstream, with everything getting commited + try (var tx = Transaction.openRoot()) { + assertThat(journal.hasOngoingTransaction()).isEqualTo(false); + + try (var tx2 = Transaction.open(tx)) { + try (var tx3 = Transaction.open(tx2)) { + journal.updateSnapshots(tx3); + assertThat(journal.hasOngoingTransaction()).isEqualTo(true); + tx3.commit(); + } + + assertThat(journal.hasOngoingTransaction()).isEqualTo(true); + tx2.commit(); + assertThat(journal.hasOngoingTransaction()).isEqualTo(true); + } + + assertThat(journal.hasOngoingTransaction()).isEqualTo(true); + tx.commit(); + assertThat(journal.hasOngoingTransaction()).isEqualTo(false); + } + + assertThat(journal.hasOngoingTransaction()).isEqualTo(false); + + // taking part only somewhere in downstream, with everything getting commited except root + try (var tx = Transaction.openRoot()) { + assertThat(journal.hasOngoingTransaction()).isEqualTo(false); + + try (var tx2 = Transaction.open(tx)) { + try (var tx3 = Transaction.open(tx2)) { + journal.updateSnapshots(tx3); + assertThat(journal.hasOngoingTransaction()).isEqualTo(true); + tx3.commit(); + } + + assertThat(journal.hasOngoingTransaction()).isEqualTo(true); + tx2.commit(); + assertThat(journal.hasOngoingTransaction()).isEqualTo(true); + } + + assertThat(journal.hasOngoingTransaction()).isEqualTo(true); + } + + assertThat(journal.hasOngoingTransaction()).isEqualTo(false); + } + @Test void testNullSnapshots() { class VoidJournal extends SnapshotJournal { From 09211c77499fae4028fa885874f07f521008d088 Mon Sep 17 00:00:00 2001 From: DBotThePony Date: Mon, 10 Aug 2026 14:05:59 +0700 Subject: [PATCH 2/5] Update newly added doc to markdown --- .../neoforge/transfer/transaction/SnapshotJournal.java | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/main/java/net/neoforged/neoforge/transfer/transaction/SnapshotJournal.java b/src/main/java/net/neoforged/neoforge/transfer/transaction/SnapshotJournal.java index a066edb53e..47d1c7e8e0 100644 --- a/src/main/java/net/neoforged/neoforge/transfer/transaction/SnapshotJournal.java +++ b/src/main/java/net/neoforged/neoforge/transfer/transaction/SnapshotJournal.java @@ -101,11 +101,7 @@ public abstract class SnapshotJournal { */ protected void onRootCommit(T originalState) {} - /** - * Signifies whenever this journal is part of any ongoing transaction. - * - * @return whenever this journal is part of any ongoing transaction - */ + /// {@return whenever this journal is part of any ongoing transaction} public final boolean hasOngoingTransaction() { return hasOngoingTransaction; } From fe7e8d620ad9ca3b736759d3ad256030c039b63e Mon Sep 17 00:00:00 2001 From: DBotThePony Date: Mon, 10 Aug 2026 15:02:47 +0700 Subject: [PATCH 3/5] Update docs for createSnapshot and revertToSnapshot, remove mentions of createSnapshot being disallowed to return nulls Nullability of createSnapshot solely depends on nullability of generic type argument `T` --- .../transfer/transaction/SnapshotJournal.java | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/main/java/net/neoforged/neoforge/transfer/transaction/SnapshotJournal.java b/src/main/java/net/neoforged/neoforge/transfer/transaction/SnapshotJournal.java index 47d1c7e8e0..a2044cf29b 100644 --- a/src/main/java/net/neoforged/neoforge/transfer/transaction/SnapshotJournal.java +++ b/src/main/java/net/neoforged/neoforge/transfer/transaction/SnapshotJournal.java @@ -38,10 +38,7 @@ import org.jspecify.annotations.Nullable; * @param The objects that this journal uses to record its state snapshots. */ public abstract class SnapshotJournal { - /** - * Used for entries of {@link #snapshots} that do not correspond to a snapshot. - * {@code null} corresponds to a snapshot that happens to be {@code null}. - */ + /// Unique unit marker in snapshot stack, representing absence of created snapshot inside specific transaction private static final Object NO_SNAPSHOT = new Object(); private final ArrayList snapshots = new ArrayList<>(); @@ -51,15 +48,18 @@ public abstract class SnapshotJournal { @Nullable private T originalState = null; - /** - * Return a new nonnull object containing the current state of this journal. - * {@code null} may not be returned, or an exception will be thrown! - */ + /// {@return new *independent* state copy of this journal} + /// This value later will be passed to either [SnapshotJournal#revertToSnapshot] (on transaction abortion, directly or due to parent being aborted) + /// or [SnapshotJournal#onRootCommit] (when root transaction concludes successfully). + /// + /// Called only *once* per transaction, which can happen inside other transaction. + /// + /// One may settle for some form of partial independence, the only requirement is that value returned by this + /// method will be sufficient to rollback (via [SnapshotJournal#revertToSnapshot]) this journal to point in time this method was called. protected abstract T createSnapshot(); - /** - * Roll back to a state previously created by {@link #createSnapshot}. - */ + /// Roll back to a state previously created by [SnapshotJournal#createSnapshot]. + /// Called when current or parental transaction is aborted, in which this journal took part of. protected abstract void revertToSnapshot(T snapshot); /** From c2ce465c0f489f0f9611349768cd3cc2b776550c Mon Sep 17 00:00:00 2001 From: DBotThePony Date: Tue, 11 Aug 2026 00:36:57 +0700 Subject: [PATCH 4/5] Implement hasOngoingTransaction as snapshot count tracking, update docs --- .../transfer/transaction/SnapshotJournal.java | 33 +++++-------------- 1 file changed, 8 insertions(+), 25 deletions(-) diff --git a/src/main/java/net/neoforged/neoforge/transfer/transaction/SnapshotJournal.java b/src/main/java/net/neoforged/neoforge/transfer/transaction/SnapshotJournal.java index a2044cf29b..28832d97ae 100644 --- a/src/main/java/net/neoforged/neoforge/transfer/transaction/SnapshotJournal.java +++ b/src/main/java/net/neoforged/neoforge/transfer/transaction/SnapshotJournal.java @@ -38,12 +38,11 @@ import org.jspecify.annotations.Nullable; * @param The objects that this journal uses to record its state snapshots. */ public abstract class SnapshotJournal { - /// Unique unit marker in snapshot stack, representing absence of created snapshot inside specific transaction + /// Used for entries of [SnapshotJournal#snapshots] that do not correspond to a snapshot. private static final Object NO_SNAPSHOT = new Object(); private final ArrayList snapshots = new ArrayList<>(); - - private boolean hasOngoingTransaction = false; + private int snapshotCount = 0; @Nullable private T originalState = null; @@ -52,14 +51,11 @@ public abstract class SnapshotJournal { /// This value later will be passed to either [SnapshotJournal#revertToSnapshot] (on transaction abortion, directly or due to parent being aborted) /// or [SnapshotJournal#onRootCommit] (when root transaction concludes successfully). /// - /// Called only *once* per transaction, which can happen inside other transaction. - /// /// One may settle for some form of partial independence, the only requirement is that value returned by this /// method will be sufficient to rollback (via [SnapshotJournal#revertToSnapshot]) this journal to point in time this method was called. protected abstract T createSnapshot(); /// Roll back to a state previously created by [SnapshotJournal#createSnapshot]. - /// Called when current or parental transaction is aborted, in which this journal took part of. protected abstract void revertToSnapshot(T snapshot); /** @@ -103,7 +99,7 @@ public abstract class SnapshotJournal { /// {@return whenever this journal is part of any ongoing transaction} public final boolean hasOngoingTransaction() { - return hasOngoingTransaction; + return snapshotCount > 0; } /** @@ -127,7 +123,7 @@ public abstract class SnapshotJournal { var transactionImpl = (Transaction) transaction; transactionImpl.validateOpen(); - hasOngoingTransaction = true; + snapshotCount++; snapshots.set(currentDepth, createSnapshot()); transactionImpl.journalsToClose.add(this); @@ -146,6 +142,7 @@ public abstract class SnapshotJournal { if (wasAborted) { // If the transaction was aborted, we just revert to the state of the snapshot. revertToSnapshot(snapshot); + snapshotCount--; releaseSnapshot(snapshot); } else if (currentDepth == 0) { // The transaction is the root. @@ -158,6 +155,8 @@ public abstract class SnapshotJournal { // In this case we just wait for the already-registered callback to run. releaseSnapshot(snapshot); } + + snapshotCount = 0; } else if (snapshots.get(currentDepth - 1) == NO_SNAPSHOT) { // No snapshot yet, so move the snapshot one depth up. snapshots.set(currentDepth - 1, snapshot); @@ -166,23 +165,7 @@ public abstract class SnapshotJournal { } else { // There is already an older snapshot at the depth above, just release the newer one. releaseSnapshot(snapshot); - } - - // perform callbacks before declaring this snapshot being no longer part of ongoing transaction - if (currentDepth == 0) { - // root - hasOngoingTransaction = false; - } else { - // snapshot list may have gaps, so we must walk upwards until we hit valid snapshot - for (int i = currentDepth - 1; i >= 0; i--) { - if (snapshots.get(i) != NO_SNAPSHOT) { - hasOngoingTransaction = true; - return; - } - } - - // no snapshot state, we don't belong to a transaction - hasOngoingTransaction = false; + snapshotCount--; } } From cbedce8eadfb1f63acf708be499f34b05f8927b2 Mon Sep 17 00:00:00 2001 From: DBotThePony Date: Tue, 11 Aug 2026 08:53:52 +0700 Subject: [PATCH 5/5] Specify in docs behavior of hasOngoingTransaction inside callbacks --- .../transfer/transaction/SnapshotJournal.java | 21 +++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/src/main/java/net/neoforged/neoforge/transfer/transaction/SnapshotJournal.java b/src/main/java/net/neoforged/neoforge/transfer/transaction/SnapshotJournal.java index 28832d97ae..c86c88be62 100644 --- a/src/main/java/net/neoforged/neoforge/transfer/transaction/SnapshotJournal.java +++ b/src/main/java/net/neoforged/neoforge/transfer/transaction/SnapshotJournal.java @@ -53,20 +53,27 @@ public abstract class SnapshotJournal { /// /// One may settle for some form of partial independence, the only requirement is that value returned by this /// method will be sufficient to rollback (via [SnapshotJournal#revertToSnapshot]) this journal to point in time this method was called. + /// + /// [SnapshotJournal#hasOngoingTransaction] will return `true` inside this callback, and will continue to return `true` + /// until all snapshots are either reverted to or [SnapshotJournal#onRootCommit] is called. protected abstract T createSnapshot(); /// Roll back to a state previously created by [SnapshotJournal#createSnapshot]. + /// + /// [SnapshotJournal#hasOngoingTransaction] will return `true` inside this callback, and, given there + /// are no snapshots left, will return `false` right after this callback returns. protected abstract void revertToSnapshot(T snapshot); - /** - * Signals that the snapshot will not be used anymore, and is safe to cache for future calls to {@link #createSnapshot}, - * or discard entirely. - */ + /// Signals that the snapshot will not be used anymore, and is safe to cache for future calls to [SnapshotJournal#createSnapshot], + /// or discard entirely. + /// + /// [SnapshotJournal#hasOngoingTransaction] will return `false` inside this callback protected void releaseSnapshot(T snapshot) {} /** * Called after the root transaction was successfully committed, * to perform irreversible actions such as {@code setChanged()} or neighbor updates. + * {@link #hasOngoingTransaction} will return {@code false} inside this callback. * *

When a root transaction is being closed, * all journals for which {@code onRootCommit} will be called are stored in a global thread-local queue. @@ -98,6 +105,12 @@ public abstract class SnapshotJournal { protected void onRootCommit(T originalState) {} /// {@return whenever this journal is part of any ongoing transaction} + /// + /// Check specific callbacks docs to see when this method returns `true`. + /// + /// @see SnapshotJournal#createSnapshot + /// @see SnapshotJournal#revertToSnapshot + /// @see SnapshotJournal#onRootCommit public final boolean hasOngoingTransaction() { return snapshotCount > 0; }