This is an automated email from the ASF dual-hosted git repository. kenhuuu pushed a commit to branch remove-txopen in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
commit ca8e57bc9158ad9aec03d4a4d38578f7822d41e6 Author: Ken Hu <[email protected]> AuthorDate: Thu Jun 18 20:13:52 2026 -0700 TINKERPOP-3252 Replace Transaction.open() with idempotent begin() open() and begin() were redundant ways to start a transaction, and the strict "throw if already open" contract was incompatible with the embedded AUTO behavior: a read opens the transaction implicitly, so a later explicit begin() would throw even though the caller did nothing wrong. Collapsing to a single idempotent begin() makes explicit and implicit opens compose, and gives one consistent transaction-start verb across embedded, remote, and all GLVs. close() is made idempotent for the same reason — so the common try-with-resources / double-close patterns are safe rather than surprising. The base AbstractTransaction.begin() now opens via a guarded doOpen() so the contract holds for every provider (not just TinkerGraph) and MANUAL mode is no longer broken in the base class. Assisted-by: Claude Code:claude-opus-4-8 --- CHANGELOG.asciidoc | 2 + docs/src/reference/the-traversal.asciidoc | 35 ++++++-- docs/src/upgrade/release-4.x.x.asciidoc | 16 ++++ .../tinkerpop/gremlin/structure/Transaction.java | 29 +++---- .../structure/util/AbstractTransaction.java | 27 +++--- .../Driver/Remote/TransactionRemoteConnection.cs | 4 +- .../src/Gremlin.Net/Driver/RemoteTransaction.cs | 96 +++++++++++++--------- .../Driver/TransactionTests.cs | 41 ++++++++- .../driver/remote/HttpRemoteTransaction.java | 65 ++++++++------- gremlin-go/driver/error_codes.go | 2 +- gremlin-go/driver/resources/error-messages/en.json | 2 +- gremlin-go/driver/transaction.go | 58 +++++++------ gremlin-go/driver/transaction_test.go | 38 +++++++-- .../gremlin-javascript/lib/process/transaction.ts | 57 +++++++------ .../test/integration/transaction-tests.js | 52 ++++++++---- .../python/gremlin_python/driver/transaction.py | 62 ++++++++------ .../tests/integration/driver/test_transaction.py | 42 ++++++++-- .../server/handler/HttpGremlinEndpointHandler.java | 4 +- .../server/transaction/UnmanagedTransaction.java | 2 +- .../GremlinDriverTransactionIntegrateTest.java | 37 +++++++-- .../process/traversal/CoreTraversalTest.java | 4 +- .../gremlin/structure/TransactionTest.java | 54 +++++++----- .../tinkergraph/structure/TinkerTransaction.java | 7 -- .../structure/TinkerTransactionGraphTest.java | 22 +++++ 24 files changed, 496 insertions(+), 262 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 594070c05c..3a167d44ac 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -25,6 +25,8 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima [[release-4-0-0]] === TinkerPop 4.0.0 (Release Date: NOT OFFICIALLY RELEASED YET) +* Removed `Transaction.open()` in favor of `begin()`, which is now the single transaction-start primitive across embedded and remote contexts. +* Changed `begin()` and `close()` to be idempotent and calling it when a transaction is already in that state no longer throws. * Added configurable CORS `allowedOrigins` setting to Gremlin Server; warns when wildcard origin is used alongside authentication. * Fixed `ByteBuf` leak in `GraphBinaryMessageSerializerV4` when serialization throws an `IOException`. * Changed `Tree` to no longer extend `HashMap`; it is now a final class with a tree-shaped API (`childAt`, `hasChild`, `contains`, `findSubtree`, `getOrCreateChild`, `getNodesAtDepth`, `getLeafNodes`, `nodeCount`) and is no longer a `Map`. diff --git a/docs/src/reference/the-traversal.asciidoc b/docs/src/reference/the-traversal.asciidoc index 46c53047a9..bed4b8ce34 100644 --- a/docs/src/reference/the-traversal.asciidoc +++ b/docs/src/reference/the-traversal.asciidoc @@ -89,9 +89,10 @@ in relation to the usage convention and graph provider caveats alluded to earlie Focusing on remote contexts first, note that it is still possible to issue traversals from `g`, but those will operate as implicit transactions (as opposed to the explicit transaction opened by `gtx`) and simply behave as -self-contained units of work (i.e. one traversal is one implicit transaction). Each explicit transaction requires its -own `Transaction` object. Multiple `begin()` calls on the same `Transaction` object are not permitted and will throw -an `IllegalStateException`: +self-contained units of work (i.e. one traversal is one implicit transaction). Each independent explicit transaction +requires its own `Transaction` object obtained from `g.tx()`. Calling `begin()` more than once on the same +`Transaction` object is idempotent - it does not start a new transaction and does not throw, returning a +`GraphTraversalSource` bound to the already-open transaction: [source,java] ---- @@ -213,8 +214,25 @@ occurs. `Transaction.READ_WRITE_BEHAVIOR` contains pre-defined `Consumer` functi method. It has two options: * `AUTO` - automatic transactions where the transaction is started implicitly to the read or write operation -* `MANUAL` - manual transactions where it is up to the user to explicitly open a transaction, throwing an exception -if the transaction is not open +* `MANUAL` - manual transactions where it is up to the user to explicitly begin a transaction with `begin()`, +throwing an exception if the transaction is not open + +The `begin()` method is idempotent with respect to an open transaction: calling it when a transaction is already open +does not start a new transaction and does not throw - it returns a `TraversalSource` bound to the transaction that is +already open. This behavior is what allows `begin()` to coexist with `AUTO`. Under `AUTO`, a read or write implicitly +opens a transaction, so an explicit `begin()` issued afterward would otherwise be operating on an already-open +transaction; because `begin()` is idempotent, that call is safe rather than an error. Likewise, `close()` is +idempotent - closing a transaction that is not open is a no-op. + +How `begin()` behaves once a transaction has been closed depends on the transaction model, and the two reference +models differ here intentionally: + +* *Embedded* transactions are typically thread-bound and reusable. After a `commit()` or `rollback()`, the same +`Transaction` is reset and a subsequent `begin()` (or an implicit `AUTO` open on the next read or write) starts a new +transaction on it. This reusability is required for `AUTO` to keep working after each transaction completes. +* *Remote* transactions are single-use. A `Transaction` obtained from `g.tx()` represents one transaction; once it has +been committed or rolled back it cannot be reused, and calling `begin()` on it throws. Start another transaction by +obtaining a fresh `Transaction` from `g.tx()`. Providing a `Consumer` function to `onClose` allows configuration of how a transaction is handled when `Transaction.close()` is called. `Transaction.CLOSE_BEHAVIOR` has several pre-defined options that can be supplied to @@ -261,8 +279,8 @@ gremlin> g.tx().isOpen() ==>false gremlin> g.addV("person").property("name","marko") <6> Open a transaction before attempting to read/write the transaction -gremlin> g.tx().open() <7> -==>null +gremlin> g.tx().begin() <7> +==>graphtraversalsource[tinkertransactiongraph[vertices:1 edges:0], standard] gremlin> g.addV("person").property("name","marko") <8> ==>v[1] gremlin> g.tx().commit() @@ -276,7 +294,8 @@ or other read operations executed in the context of that open transaction. <4> Calling `commit` finalizes the transaction. <5> Change transaction behavior to require manual control. <6> Adding a vertex now results in failure because the transaction was not explicitly opened. -<7> Explicitly open a transaction. +<7> Explicitly begin a transaction with `begin()`, which returns a `GraphTraversalSource` bound to it. Calling +`begin()` when a transaction is already open is idempotent and will not throw. <8> Adding a vertex now succeeds as the transaction was manually opened. NOTE: It may be important to consult the documentation of the `Graph` implementation you are using when it comes to the diff --git a/docs/src/upgrade/release-4.x.x.asciidoc b/docs/src/upgrade/release-4.x.x.asciidoc index b2a3bdf09e..19d3186167 100644 --- a/docs/src/upgrade/release-4.x.x.asciidoc +++ b/docs/src/upgrade/release-4.x.x.asciidoc @@ -151,6 +151,22 @@ Key behaviors consistent across all GLVs: See the <<gremlin-drivers-variants,Gremlin Drivers and Variants>> reference documentation for language-specific syntax and examples. +==== `Transaction.open()` Replaced by `begin()` + +The `open()` method has been removed from the `Transaction` API. Use `begin()` instead, which is now the single +transaction-start method for both embedded and remote contexts. Replace any `tx.open()` or `g.tx().open()` calls with +`begin()`. This is a compile-time break and is straightforward to find and fix. + +In addition, `begin()` is idempotent: calling it when a transaction is already open does not start a new transaction +and does not throw, returning a `TraversalSource` bound to the existing transaction. For embedded graphs this replaces +the previous behavior where opening an already-open transaction threw `transactionAlreadyOpen()`; that exception +factory (`Transaction.Exceptions.transactionAlreadyOpen()`) has been removed, so review and remove any code that +catches it or relies on a second open failing. For the semantics of `begin()` and how it interacts with +`AUTO`/`MANUAL` transactions, see the +link:https://tinkerpop.apache.org/docs/x.y.z/reference/#transactions[Traversal Transactions] reference documentation. + +See: link:https://issues.apache.org/jira/browse/TINKERPOP-3252[TINKERPOP-3252] + ==== Transaction Default Close Behavior Changed The default behavior of `close()` on a remote transaction has been changed from `commit` to `rollback` across all diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/Transaction.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/Transaction.java index 45e6273d85..b04dea3537 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/Transaction.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/Transaction.java @@ -50,11 +50,6 @@ public interface Transaction extends AutoCloseable { } //////////////// - /** - * Opens a transaction. - */ - public void open(); - /** * Commits a transaction. This method may optionally throw {@link TransactionException} on error. Providers should * consider wrapping their transaction exceptions in this TinkerPop exception as it will lead to better error @@ -81,16 +76,21 @@ public interface Transaction extends AutoCloseable { } /** - * Starts a transaction in the context of a {@link GraphTraversalSource} instance. It is up to the - * {@link Transaction} implementation to decide what this means and up to users to be aware of that meaning. + * Starts a transaction in the context of a {@link GraphTraversalSource} instance and returns that + * transaction-bound source. See {@link #begin(Class)} for the full contract. */ public default <T extends TraversalSource> T begin() { return (T) begin(GraphTraversalSource.class); } /** - * Starts a transaction in the context of a particular {@link TraversalSource} instance. It is up to the - * {@link Transaction} implementation to decide what this means and up to users to be aware of that meaning. + * Starts a transaction in the context of a particular {@link TraversalSource} instance and returns a + * {@link TraversalSource} bound to it. If a transaction is not already open for this {@link Transaction}, one + * is started; if a transaction is already open, this method is idempotent - it does not start a new + * transaction and does not throw, returning a source bound to the open transaction. The identity of the + * returned source across calls is unspecified and must not be relied upon. How the returned + * {@link TraversalSource} is bound to the transaction's context is up to the implementation and up to users to + * be aware of that meaning. */ public <T extends TraversalSource> T begin(final Class<T> traversalSourceClass); @@ -153,10 +153,6 @@ public interface Transaction extends AutoCloseable { private Exceptions() { } - public static IllegalStateException transactionAlreadyOpen() { - return new IllegalStateException("Stop the current transaction before opening another"); - } - public static IllegalStateException transactionMustBeOpenToReadWrite() { return new IllegalStateException("Open a transaction before attempting to read/write the transaction"); } @@ -224,7 +220,7 @@ public interface Transaction extends AutoCloseable { AUTO { @Override public void accept(final Transaction transaction) { - if (!transaction.isOpen()) transaction.open(); + if (!transaction.isOpen()) transaction.begin(); } }, @@ -240,11 +236,6 @@ public interface Transaction extends AutoCloseable { } public static final Transaction NO_OP = new Transaction() { - @Override - public void open() { - throw new UnsupportedOperationException("This Transaction implementation is a no-op for all methods"); - } - @Override public void commit() { throw new UnsupportedOperationException("This Transaction implementation is a no-op for all methods"); diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/util/AbstractTransaction.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/util/AbstractTransaction.java index bd54e08319..c9575ba771 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/util/AbstractTransaction.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/util/AbstractTransaction.java @@ -41,8 +41,8 @@ public abstract class AbstractTransaction implements Transaction { } /** - * Called within {@link #open} if it is determined that the transaction is not yet open given {@link #isOpen}. - * Implementers should assume the transaction is not yet started and should thus open one. + * Called within {@link #begin(Class)} if it is determined that the transaction is not yet open given + * {@link #isOpen}. Implementers should assume the transaction is not yet started and should thus open one. */ protected abstract void doOpen(); @@ -84,17 +84,6 @@ public abstract class AbstractTransaction implements Transaction { */ protected abstract void doClose(); - /** - * {@inheritDoc} - */ - @Override - public void open() { - if (isOpen()) - throw Transaction.Exceptions.transactionAlreadyOpen(); - else - doOpen(); - } - /** * {@inheritDoc} */ @@ -123,8 +112,20 @@ public abstract class AbstractTransaction implements Transaction { throw Transaction.Exceptions.threadedTransactionsNotSupported(); } + /** + * {@inheritDoc} + * <p> + * Starts a transaction if one is not already open for this {@code Transaction} (delegating to + * {@link #doOpen()} under an {@link #isOpen()} guard) and returns a {@link TraversalSource} bound to it. + * This method is idempotent with respect to the transaction: calling it while a transaction is already open + * does not start a new transaction and does not throw - it simply returns a traversal source bound to the + * open transaction. The identity of the returned source across calls is unspecified; callers must not rely + * on reference identity. + */ @Override public <T extends TraversalSource> T begin(final Class<T> traversalSourceClass) { + if (!isOpen()) + doOpen(); return graph.traversal(traversalSourceClass); } diff --git a/gremlin-dotnet/src/Gremlin.Net/Driver/Remote/TransactionRemoteConnection.cs b/gremlin-dotnet/src/Gremlin.Net/Driver/Remote/TransactionRemoteConnection.cs index 9fa1e57441..d63ec834f7 100644 --- a/gremlin-dotnet/src/Gremlin.Net/Driver/Remote/TransactionRemoteConnection.cs +++ b/gremlin-dotnet/src/Gremlin.Net/Driver/Remote/TransactionRemoteConnection.cs @@ -97,8 +97,8 @@ namespace Gremlin.Net.Driver.Remote /// <inheritdoc /> public RemoteTransaction Tx(GraphTraversalSource g) { - // Return the existing transaction. Calling BeginAsync() on it will throw - // "Transaction already started" since it's already open. + // Return the existing transaction. Calling BeginAsync() on it again is idempotent + // (it is already open) and returns a source bound to the same transaction. return _transaction; } diff --git a/gremlin-dotnet/src/Gremlin.Net/Driver/RemoteTransaction.cs b/gremlin-dotnet/src/Gremlin.Net/Driver/RemoteTransaction.cs index f7f0f93fbc..7e2ef2f873 100644 --- a/gremlin-dotnet/src/Gremlin.Net/Driver/RemoteTransaction.cs +++ b/gremlin-dotnet/src/Gremlin.Net/Driver/RemoteTransaction.cs @@ -73,68 +73,82 @@ namespace Gremlin.Net.Driver /// <summary> /// Starts the transaction and returns a transaction-bound <see cref="GraphTraversalSource"/>. + /// <para> + /// This method is idempotent: calling it while a transaction is already open does not send a second + /// begin to the server and does not throw - it reuses the existing transaction ID and returns a source + /// bound to the same transaction. A transaction is single-use, so calling it after the transaction has + /// been closed (commit/rollback/failed begin) throws. + /// </para> /// </summary> /// <param name="cancellationToken">The token to cancel the operation.</param> /// <returns>A <see cref="GraphTraversalSource"/> bound to this transaction.</returns> - /// <exception cref="InvalidOperationException">Thrown if the transaction is already started.</exception> + /// <exception cref="InvalidOperationException">Thrown if the transaction has already been closed.</exception> public async Task<GraphTraversalSource> BeginAsync(CancellationToken cancellationToken = default) { - if (_isOpen || _failed) + if (_failed) { - throw new InvalidOperationException("Transaction already started"); + throw new InvalidOperationException( + "Transaction is closed and cannot be reused; begin a new transaction"); } - var requestMsg = RequestMessage.Build("g.tx().begin()") - .AddG(_traversalSource) - .Create(); - - await _submitLock.WaitAsync(cancellationToken).ConfigureAwait(false); - try + // idempotent: if a transaction is already open, reuse the existing transactionId without sending a + // second begin to the server, and return a source bound to the same transaction + if (!_isOpen) { - List<object> results; + var requestMsg = RequestMessage.Build("g.tx().begin()") + .AddG(_traversalSource) + .Create(); + + await _submitLock.WaitAsync(cancellationToken).ConfigureAwait(false); try { - var resultSet = await _client.SubmitAsync<object>(requestMsg, cancellationToken) - .ConfigureAwait(false); - results = await resultSet.ToListAsync(cancellationToken).ConfigureAwait(false); - } - catch - { - _failed = true; - throw; - } + List<object> results; + try + { + var resultSet = await _client.SubmitAsync<object>(requestMsg, cancellationToken) + .ConfigureAwait(false); + results = await resultSet.ToListAsync(cancellationToken).ConfigureAwait(false); + } + catch + { + _failed = true; + throw; + } - if (results.Count == 0) - { - _failed = true; - throw new InvalidOperationException("Server did not return transaction ID"); - } + if (results.Count == 0) + { + _failed = true; + throw new InvalidOperationException("Server did not return transaction ID"); + } - if (results[0] is Dictionary<object, object> resultMap && - resultMap.TryGetValue("transactionId", out var txIdObj) && - txIdObj is string txId && !string.IsNullOrEmpty(txId)) - { - _transactionId = txId; + if (results[0] is Dictionary<object, object> resultMap && + resultMap.TryGetValue("transactionId", out var txIdObj) && + txIdObj is string txId && !string.IsNullOrEmpty(txId)) + { + _transactionId = txId; + } + else + { + _failed = true; + throw new InvalidOperationException("Server did not return transaction ID in expected format"); + } + + // assign _txConnection before publishing _isOpen=true so any thread that observes the + // transaction as open is guaranteed to also see a non-null _txConnection + _txConnection = new TransactionRemoteConnection(_client, _traversalSource, _transactionId, this); + _isOpen = true; + (_client as GremlinClient)?.TrackTransaction(this); } - else + finally { - _failed = true; - throw new InvalidOperationException("Server did not return transaction ID in expected format"); + _submitLock.Release(); } - - _isOpen = true; - (_client as GremlinClient)?.TrackTransaction(this); - } - finally - { - _submitLock.Release(); } - _txConnection = new TransactionRemoteConnection(_client, _traversalSource, _transactionId, this); return new GraphTraversalSource( new List<ITraversalStrategy>(), new GremlinLang(), - _txConnection); + _txConnection!); } /// <summary> diff --git a/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Driver/TransactionTests.cs b/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Driver/TransactionTests.cs index 57df672be9..79f1373079 100644 --- a/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Driver/TransactionTests.cs +++ b/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Driver/TransactionTests.cs @@ -149,13 +149,25 @@ namespace Gremlin.Net.IntegrationTest.Driver } [Fact] - public async Task ShouldThrowOnDoubleBegin() + public async Task ShouldBeIdempotentOnDoubleBegin() { using var client = CreateClient(); + await DropGraph(client); var tx = client.Transact("gtx"); await tx.BeginAsync(); + var txId = tx.TransactionId; - await Assert.ThrowsAsync<InvalidOperationException>(() => tx.BeginAsync()); + // BeginAsync while already open is idempotent: it does not throw and does not start a new + // server-side transaction (the transactionId is unchanged) + var gtx = await tx.BeginAsync(); + Assert.True(tx.IsOpen); + Assert.Equal(txId, tx.TransactionId); + + // the source from the second begin works within the same transaction + await gtx.AddV("person").Property("name", "double_begin").Promise(t => t.Iterate()); + Assert.Equal(1L, await gtx.V().Has("name", "double_begin").Count().Promise(t => t.Next())); + + await tx.RollbackAsync(); } [Fact] @@ -205,6 +217,22 @@ namespace Gremlin.Net.IntegrationTest.Driver Assert.Equal(0L, await GetCount(client, "person")); } + [Fact] + public async Task ShouldBeIdempotentOnDoubleDispose() + { + using var client = CreateClient(); + await DropGraph(client); + var tx = client.Transact("gtx"); + await tx.BeginAsync(); + await tx.RollbackAsync(); + Assert.False(tx.IsOpen); + + // disposing (closing) an already-closed transaction is a safe no-op (no exception) + await tx.DisposeAsync(); + await tx.DisposeAsync(); + Assert.False(tx.IsOpen); + } + [Fact] public async Task ShouldIsolateConcurrentTransactions() { @@ -320,7 +348,7 @@ namespace Gremlin.Net.IntegrationTest.Driver } [Fact] - public async Task ShouldThrowOnBeginFromGtxTx() + public async Task ShouldBeIdempotentOnBeginFromGtxTx() { using var client = CreateClient(); await DropGraph(client); @@ -329,9 +357,14 @@ namespace Gremlin.Net.IntegrationTest.Driver var tx = g.Tx(); var gtx = await tx.BeginAsync(); + var txId = tx.TransactionId; var sameTx = gtx.Tx(); - await Assert.ThrowsAsync<InvalidOperationException>(() => sameTx.BeginAsync()); + // BeginAsync on the same (already open) transaction obtained via gtx.Tx() is idempotent: it + // does not start a new server-side transaction, so it stays bound to the same transaction id + await sameTx.BeginAsync(); + Assert.True(sameTx.IsOpen); + Assert.Equal(txId, sameTx.TransactionId); await tx.RollbackAsync(); } diff --git a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/remote/HttpRemoteTransaction.java b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/remote/HttpRemoteTransaction.java index 61e2d6bbcd..4c56b36796 100644 --- a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/remote/HttpRemoteTransaction.java +++ b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/remote/HttpRemoteTransaction.java @@ -97,45 +97,47 @@ public class HttpRemoteTransaction implements RemoteTransaction { this.cluster = pinnedClient.getCluster(); } - /** - * Not supported for remote transactions. Use {@link #begin(Class)} instead. - * - * @throws UnsupportedOperationException always - */ - @Override - public void open() { - begin(); - } - /** * Starts a transaction and returns a traversal source bound to it. * <p> - * This method sends {@code g.tx().begin()} to the server, which returns - * the transaction ID. All subsequent requests will include this ID. + * When this transaction has not yet been started, this method sends {@code g.tx().begin()} to the server, + * which returns the transaction ID that all subsequent requests will include. This method is idempotent: if + * the transaction is already open, it does not send a second begin to the server and does not throw - it + * reuses the existing transaction ID and returns a traversal source bound to the same transaction. A remote + * transaction is single-use, so once it has been closed (via {@code commit()}, {@code rollback()}, timeout, + * or a failed begin) it cannot be reused and this method throws. * * @param traversalSourceClass the class of the traversal source to create * @param <T> the type of the traversal source - * @return a new traversal source bound to this transaction - * @throws IllegalStateException if the transaction is already started + * @return a traversal source bound to this transaction + * @throws IllegalStateException if the transaction has already been closed * @throws RuntimeException if the transaction fails to begin */ @Override public <T extends TraversalSource> T begin(final Class<T> traversalSourceClass) { - if (state != TransactionState.NOT_STARTED) { - throw new IllegalStateException("Transaction already started"); - } - cluster.trackTransaction(this); - - try { - // Send begin - no txId attached yet - final ResultSet rs = submitInternal("g.tx().begin()"); - - // Server returns the transaction ID - this.transactionId = extractTransactionId(rs); - this.state = TransactionState.OPEN; - } catch (Exception e) { - cleanUp(); - throw new RuntimeException("Failed to begin transaction: " + e.getMessage(), e); + switch (state) { + case NOT_STARTED: + cluster.trackTransaction(this); + try { + // Send begin - no txId attached yet + final ResultSet rs = submitInternal("g.tx().begin()"); + + // Server returns the transaction ID + this.transactionId = extractTransactionId(rs); + this.state = TransactionState.OPEN; + } catch (Exception e) { + cleanUp(); + throw new RuntimeException("Failed to begin transaction: " + e.getMessage(), e); + } + break; + case OPEN: + // idempotent: a transaction is already open - reuse the existing transactionId without + // sending a second begin to the server, and return a source bound to the same transaction + break; + case CLOSED: + throw new IllegalStateException("Transaction is closed and cannot be reused; begin a new transaction"); + default: + throw new IllegalStateException("Unknown transaction state: " + state); } // Create RemoteConnection for the traversal source @@ -242,8 +244,11 @@ public class HttpRemoteTransaction implements RemoteTransaction { @Override public void close() { + // close() is idempotent: closing an already-closed transaction is a safe no-op + if (state == TransactionState.CLOSED) return; + closeConsumer.accept(this); - + // this is just for safety in case of custom closeConsumer but should normally be handled by commit/rollback cleanUp(); } diff --git a/gremlin-go/driver/error_codes.go b/gremlin-go/driver/error_codes.go index 2d3c6b8917..9e727c0460 100644 --- a/gremlin-go/driver/error_codes.go +++ b/gremlin-go/driver/error_codes.go @@ -85,7 +85,7 @@ const ( err1001ConvertArgumentChildTraversalNotFromAnonError errorCode = "E1001_BYTECODE_CHILD_T_NOT_ANON_ERROR" // transaction.go errors - err1101TransactionRepeatedOpenError errorCode = "E1101_TRANSACTION_REPEATED_OPEN_ERROR" + err1101TransactionClosedCannotReuseError errorCode = "E1101_TRANSACTION_CLOSED_CANNOT_REUSE_ERROR" err1102TransactionRollbackNotOpenedError errorCode = "E1102_TRANSACTION_ROLLBACK_NOT_OPENED_ERROR" err1103TransactionCommitNotOpenedError errorCode = "E1103_TRANSACTION_COMMIT_NOT_OPENED_ERROR" err1104TransactionRepeatedCloseError errorCode = "E1104_TRANSACTION_REPEATED_CLOSE_ERROR" diff --git a/gremlin-go/driver/resources/error-messages/en.json b/gremlin-go/driver/resources/error-messages/en.json index fc33202bf5..0c8fa6c4bd 100644 --- a/gremlin-go/driver/resources/error-messages/en.json +++ b/gremlin-go/driver/resources/error-messages/en.json @@ -47,7 +47,7 @@ "E1001_BYTECODE_CHILD_T_NOT_ANON_ERROR": "E1001: the child traversal was not spawned anonymously - use the T__ class rather than a TraversalSource to construct the child traversal", - "E1101_TRANSACTION_REPEATED_OPEN_ERROR": "E1101: transaction already started on this object", + "E1101_TRANSACTION_CLOSED_CANNOT_REUSE_ERROR": "E1101: transaction is closed and cannot be reused; begin a new transaction", "E1102_TRANSACTION_ROLLBACK_NOT_OPENED_ERROR": "E1102: cannot rollback a transaction that is not started", "E1103_TRANSACTION_COMMIT_NOT_OPENED_ERROR": "E1103: cannot commit a transaction that is not started", "E1104_TRANSACTION_REPEATED_CLOSE_ERROR": "E1104: cannot close a transaction that has previously been closed", diff --git a/gremlin-go/driver/transaction.go b/gremlin-go/driver/transaction.go index 9f7846031c..9dfd12fb92 100644 --- a/gremlin-go/driver/transaction.go +++ b/gremlin-go/driver/transaction.go @@ -44,38 +44,48 @@ type Transaction struct { mutex sync.Mutex } +// Begin starts the transaction and returns a transaction-bound GraphTraversalSource. +// +// Begin is idempotent: calling it while a transaction is already open does not send a second +// begin to the server and does not return an error - it reuses the existing transaction ID and +// returns a source bound to the same transaction. A transaction is single-use, so calling Begin +// after it has been closed (commit/rollback/failed begin) returns an error. func (t *Transaction) Begin() (*GraphTraversalSource, error) { t.mutex.Lock() defer t.mutex.Unlock() - if t.isOpen || t.failed { - return nil, newError(err1101TransactionRepeatedOpenError) - } - - // Submit g.tx().begin() via the Client to obtain a server-generated transactionId - rs, err := t.client.SubmitWithOptions("g.tx().begin()", - RequestOptions{}) - if err != nil { - t.failed = true - return nil, newError(err1105TransactionBeginFailedError, err) - } - - results, err := rs.All() - if err != nil { - t.failed = true - return nil, newError(err1105TransactionBeginFailedError, err) + if t.failed { + return nil, newError(err1101TransactionClosedCannotReuseError) } - txId, err := extractTransactionId(results) - if err != nil { - t.failed = true - return nil, err + // idempotent: if a transaction is already open, reuse the existing transactionId without + // sending a second begin to the server, and return a source bound to the same transaction + if !t.isOpen { + // Submit g.tx().begin() via the Client to obtain a server-generated transactionId + rs, err := t.client.SubmitWithOptions("g.tx().begin()", + RequestOptions{}) + if err != nil { + t.failed = true + return nil, newError(err1105TransactionBeginFailedError, err) + } + + results, err := rs.All() + if err != nil { + t.failed = true + return nil, newError(err1105TransactionBeginFailedError, err) + } + + txId, err := extractTransactionId(results) + if err != nil { + t.failed = true + return nil, err + } + + t.transactionId = txId + t.isOpen = true + t.client.trackTransaction(t) } - t.transactionId = txId - t.isOpen = true - t.client.trackTransaction(t) - // Create a transaction-bound remote connection that injects transactionId txDRC := &transactionRemoteConnection{ transaction: t, diff --git a/gremlin-go/driver/transaction_test.go b/gremlin-go/driver/transaction_test.go index a449913809..6d354e3ae9 100644 --- a/gremlin-go/driver/transaction_test.go +++ b/gremlin-go/driver/transaction_test.go @@ -187,10 +187,16 @@ func TestTransactionDoubleBegin(t *testing.T) { tx := client.Transact() _, err := tx.Begin() assert.Nil(t, err) + txId := tx.TransactionId() + // Begin() while already open is idempotent: it does not error and does not start a new + // server-side transaction (the transactionId is unchanged) _, err = tx.Begin() - assert.NotNil(t, err) - assert.Contains(t, err.Error(), "E1101") + assert.Nil(t, err) + assert.True(t, tx.IsOpen()) + assert.Equal(t, txId, tx.TransactionId()) + + tx.Rollback() } func TestTransactionCommitWhenNotOpen(t *testing.T) { @@ -409,7 +415,7 @@ func TestTransactionReturnsSameTxFromGtxTx(t *testing.T) { assert.Nil(t, err) } -func TestTransactionBeginFromGtxTxThrows(t *testing.T) { +func TestTransactionBeginFromGtxTxIsIdempotent(t *testing.T) { client := newTxClient(t) defer client.Close() @@ -420,11 +426,15 @@ func TestTransactionBeginFromGtxTxThrows(t *testing.T) { tx := g.Tx() gtx, err := tx.Begin() assert.Nil(t, err) + txId := tx.TransactionId() + // Begin() on the same (already open) transaction obtained via gtx.Tx() is idempotent: it does + // not start a new server-side transaction, so it stays bound to the same transaction id sameTx := gtx.Tx() _, err = sameTx.Begin() - assert.NotNil(t, err) - assert.Contains(t, err.Error(), "E1101") + assert.Nil(t, err) + assert.True(t, sameTx.IsOpen()) + assert.Equal(t, txId, sameTx.TransactionId()) tx.Rollback() } @@ -483,6 +493,24 @@ func TestTransactionDoubleRollback(t *testing.T) { assert.Contains(t, err.Error(), "E1102") } +func TestTransactionDoubleClose(t *testing.T) { + client := newTxClient(t) + defer client.Close() + dropTxGraph(t, client) + + tx := client.Transact() + _, err := tx.Begin() + assert.Nil(t, err) + err = tx.Close() + assert.Nil(t, err) + assert.False(t, tx.IsOpen()) + + // Close() is idempotent: closing an already-closed transaction is a safe no-op (no error) + err = tx.Close() + assert.Nil(t, err) + assert.False(t, tx.IsOpen()) +} + func TestTransactionIsolateFromNonTx(t *testing.T) { client := newTxClient(t) defer client.Close() diff --git a/gremlin-js/gremlin-javascript/lib/process/transaction.ts b/gremlin-js/gremlin-javascript/lib/process/transaction.ts index 0296edfb47..57b77022b2 100644 --- a/gremlin-js/gremlin-javascript/lib/process/transaction.ts +++ b/gremlin-js/gremlin-javascript/lib/process/transaction.ts @@ -60,37 +60,46 @@ export class Transaction { /** * Spawns a GraphTraversalSource that is bound to a remote transaction. + * + * begin() is idempotent: calling it while a transaction is already open does not send a + * second begin to the server and does not throw - it reuses the existing transaction ID and + * returns a source bound to the same transaction. A transaction is single-use, so calling + * begin() after it has been closed (commit/rollback/failed begin) throws. * @returns {Promise<GraphTraversalSource>} */ async begin(): Promise<GraphTraversalSource> { - if (this._isOpen || this._failed) { - throw new Error('Transaction already started'); + if (this._failed) { + throw new Error('Transaction is closed and cannot be reused; begin a new transaction'); } - let result; - try { - result = await this._client.submit('g.tx().begin()', null); - } catch (e) { - this._failed = true; - throw e; - } - - const resultArray = result.toArray(); - if (!resultArray || resultArray.length === 0) { - this._failed = true; - throw new Error('Server did not return transaction ID'); - } - - const resultMap = resultArray[0]; - if (!resultMap || !(resultMap instanceof Map) || !resultMap.get('transactionId')) { - this._failed = true; - throw new Error('Server did not return transaction ID in expected format'); + // idempotent: if a transaction is already open, reuse the existing transactionId without + // sending a second begin to the server, and return a source bound to the same transaction + if (!this._isOpen) { + let result; + try { + result = await this._client.submit('g.tx().begin()', null); + } catch (e) { + this._failed = true; + throw e; + } + + const resultArray = result.toArray(); + if (!resultArray || resultArray.length === 0) { + this._failed = true; + throw new Error('Server did not return transaction ID'); + } + + const resultMap = resultArray[0]; + if (!resultMap || !(resultMap instanceof Map) || !resultMap.get('transactionId')) { + this._failed = true; + throw new Error('Server did not return transaction ID in expected format'); + } + + this._transactionId = resultMap.get('transactionId'); + this._isOpen = true; + this._client.trackTransaction(this); } - this._transactionId = resultMap.get('transactionId'); - this._isOpen = true; - this._client.trackTransaction(this); - // Create a DriverRemoteConnection bound to this transaction. The DRC // will automatically attach the transactionId to all requests. const txConnection = new DriverRemoteConnection( diff --git a/gremlin-js/gremlin-javascript/test/integration/transaction-tests.js b/gremlin-js/gremlin-javascript/test/integration/transaction-tests.js index 5abfc8436c..d24375dffc 100644 --- a/gremlin-js/gremlin-javascript/test/integration/transaction-tests.js +++ b/gremlin-js/gremlin-javascript/test/integration/transaction-tests.js @@ -110,14 +110,23 @@ describe('Transaction', function () { ); }); - it('should throw on double begin', async function () { + it('should be idempotent on double begin', async function () { const tx = client.transact(); await tx.begin(); + const txId = tx.transactionId; - await assert.rejects( - () => tx.begin(), - /Transaction already started/ - ); + // begin() while already open is idempotent: it does not throw and does not start a new + // server-side transaction (the transactionId is unchanged) + const gtx = await tx.begin(); + assert.strictEqual(tx.isOpen, true); + assert.strictEqual(tx.transactionId, txId); + + // the source from the second begin() works within the same transaction + await gtx.addV('person').property('name', 'double_begin').iterate(); + const count = await gtx.V().has('name', 'double_begin').count().next(); + assert.strictEqual(count.value, 1); + + await tx.rollback(); }); it('should throw on commit when not open', async function () { @@ -159,6 +168,17 @@ describe('Transaction', function () { assert.strictEqual(result.first(), 0); }); + it('should be idempotent on double close', async function () { + const tx = client.transact(); + await tx.begin(); + await tx.close(); + assert.strictEqual(tx.isOpen, false); + + // close() is idempotent: closing an already-closed transaction is a safe no-op + await tx.close(); + assert.strictEqual(tx.isOpen, false); + }); + it('should isolate concurrent transactions', async function () { const tx1 = client.transact(); await tx1.begin(); @@ -219,9 +239,10 @@ describe('Transaction', function () { await tx.begin(); await tx.commit(); + // a transaction is single-use: begin() after commit rejects (closed, cannot be reused) await assert.rejects( () => tx.begin(), - /Transaction already started/ + /Transaction is closed and cannot be reused/ ); }); @@ -244,9 +265,10 @@ describe('Transaction', function () { await tx.begin(); await tx.rollback(); + // a transaction is single-use: begin() after rollback rejects (closed, cannot be reused) await assert.rejects( () => tx.begin(), - /Transaction already started/ + /Transaction is closed and cannot be reused/ ); }); @@ -353,18 +375,20 @@ describe('Transaction', function () { await connection.close(); }); - it('should throw on begin from gtx.tx()', async function () { + it('should be idempotent on begin from gtx.tx()', async function () { const connection = getConnection('gtx'); const g = anon.traversal().withRemote(connection); const tx = g.tx(); const gtx = await tx.begin(); + const txId = tx.transactionId; const sameTx = gtx.tx(); - await assert.rejects( - () => sameTx.begin(), - /Transaction already started/ - ); + // begin() on the same (already open) transaction obtained via gtx.tx() is idempotent: it does + // not start a new server-side transaction, so it stays bound to the same transaction id + await sameTx.begin(); + assert.strictEqual(sameTx.isOpen, true); + assert.strictEqual(sameTx.transactionId, txId); await tx.rollback(); await connection.close(); @@ -412,10 +436,10 @@ describe('Transaction', function () { assert.strictEqual(tx.isOpen, false); assert.strictEqual(tx.transactionId, undefined); - // Cannot begin again + // a transaction is single-use: begin() after a failed begin rejects (closed, cannot be reused) await assert.rejects( () => tx.begin(), - /Transaction already started/ + /Transaction is closed and cannot be reused/ ); await nonTxClient.close(); diff --git a/gremlin-python/src/main/python/gremlin_python/driver/transaction.py b/gremlin-python/src/main/python/gremlin_python/driver/transaction.py index fbf1170cc5..857f1a2e3d 100644 --- a/gremlin-python/src/main/python/gremlin_python/driver/transaction.py +++ b/gremlin-python/src/main/python/gremlin_python/driver/transaction.py @@ -49,34 +49,42 @@ class Transaction: The returned GTS can be used to submit traversals within this transaction. Users of the driver-level API (client.transact()) may ignore the return value and use submit() directly instead. + + begin() is idempotent: calling it while a transaction is already open does not + send a second begin to the server and does not raise - it reuses the existing + transaction ID and returns a source bound to the same transaction. A transaction + is single-use, so calling begin() after it has been closed raises. """ - if self._is_open or self._failed: - raise Exception("Transaction already started") - - try: - result = self._client.submit("g.tx().begin()") - results = result.all().result() - except Exception: - self._failed = True - raise - - if not results: - self._failed = True - raise Exception("Server did not return transaction ID") - - result_map = results[0] - if isinstance(result_map, dict): - self._transaction_id = result_map.get('transactionId') - else: - self._failed = True - raise Exception("Server did not return transaction ID in expected format") - - if not self._transaction_id: - self._failed = True - raise Exception("Server returned empty transaction ID") - - self._is_open = True - self._client.track_transaction(self) + if self._failed: + raise Exception("Transaction is closed and cannot be reused; begin a new transaction") + + # idempotent: if a transaction is already open, reuse the existing transactionId without + # sending a second begin to the server, and return a source bound to the same transaction + if not self._is_open: + try: + result = self._client.submit("g.tx().begin()") + results = result.all().result() + except Exception: + self._failed = True + raise + + if not results: + self._failed = True + raise Exception("Server did not return transaction ID") + + result_map = results[0] + if isinstance(result_map, dict): + self._transaction_id = result_map.get('transactionId') + else: + self._failed = True + raise Exception("Server did not return transaction ID in expected format") + + if not self._transaction_id: + self._failed = True + raise Exception("Server returned empty transaction ID") + + self._is_open = True + self._client.track_transaction(self) # Return a GraphTraversalSource bound to this transaction via # TransactionRemoteConnection. Inline imports avoid circular dependencies diff --git a/gremlin-python/src/main/python/tests/integration/driver/test_transaction.py b/gremlin-python/src/main/python/tests/integration/driver/test_transaction.py index 77b5a04241..22b49c88fa 100644 --- a/gremlin-python/src/main/python/tests/integration/driver/test_transaction.py +++ b/gremlin-python/src/main/python/tests/integration/driver/test_transaction.py @@ -125,12 +125,22 @@ class TestTransaction(object): with pytest.raises(Exception, match="Transaction is not open"): tx.submit("g.V().count()") - def test_should_throw_on_double_begin(self, client): + def test_should_be_idempotent_on_double_begin(self, client): tx = client.transact() tx.begin() + tx_id = tx.transaction_id - with pytest.raises(Exception, match="Transaction already started"): - tx.begin() + # begin() while already open is idempotent: it does not raise and does not start a new + # server-side transaction (the transactionId is unchanged) + gtx = tx.begin() + assert tx.is_open + assert tx.transaction_id == tx_id + + # the source from the second begin() works within the same transaction + gtx.addV("person").property("name", "double_begin").iterate() + assert gtx.V().has("name", "double_begin").count().next() == 1 + + tx.rollback() def test_should_throw_on_commit_when_not_open(self, client): tx = client.transact() @@ -163,6 +173,16 @@ class TestTransaction(object): result = client.submit("g.V().hasLabel('person').count()").all().result() assert result[0] == 0 + def test_should_be_idempotent_on_double_close(self, client): + tx = client.transact() + tx.begin() + tx.close() + assert not tx.is_open + + # close() is idempotent: closing an already-closed transaction is a safe no-op + tx.close() + assert not tx.is_open + def test_should_isolate_concurrent_transactions(self, client): tx1 = client.transact() tx1.begin() @@ -291,13 +311,17 @@ class TestTransaction(object): same_tx = gtx.tx() assert same_tx is tx - def test_should_throw_on_begin_from_gtx_tx(self, client): + def test_should_be_idempotent_on_begin_from_gtx_tx(self, client): tx = client.transact() gtx = tx.begin() + tx_id = tx.transaction_id same_tx = gtx.tx() - with pytest.raises(Exception, match="Transaction already started"): - same_tx.begin() + # begin() on the same (already open) transaction obtained via gtx.tx() is idempotent: it does + # not start a new server-side transaction, so it stays bound to the same transaction id + same_tx.begin() + assert same_tx.is_open + assert same_tx.transaction_id == tx_id tx.rollback() @@ -333,7 +357,8 @@ class TestTransaction(object): tx.begin() tx.commit() - with pytest.raises(Exception, match="Transaction already started"): + # a transaction is single-use: begin() after commit raises (closed, cannot be reused) + with pytest.raises(Exception, match="Transaction is closed and cannot be reused"): tx.begin() def test_should_not_allow_begin_after_rollback(self, client): @@ -341,7 +366,8 @@ class TestTransaction(object): tx.begin() tx.rollback() - with pytest.raises(Exception, match="Transaction already started"): + # a transaction is single-use: begin() after rollback raises (closed, cannot be reused) + with pytest.raises(Exception, match="Transaction is closed and cannot be reused"): tx.begin() diff --git a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpGremlinEndpointHandler.java b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpGremlinEndpointHandler.java index 2248044a28..f579747640 100644 --- a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpGremlinEndpointHandler.java +++ b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpGremlinEndpointHandler.java @@ -478,7 +478,7 @@ public class HttpGremlinEndpointHandler extends SimpleChannelInboundHandler<Requ } /** - * Handle begin by creating an {@link UnmanagedTransaction} and submitting the open to its executor. + * Handle begin by creating an {@link UnmanagedTransaction} and submitting the transaction begin to its executor. */ private void doBegin(final Context ctx) throws Exception { final String traversalSourceName = ctx.getRequestMessage().getField(Tokens.ARGS_G); @@ -489,7 +489,7 @@ public class HttpGremlinEndpointHandler extends SimpleChannelInboundHandler<Requ ctx.setTransactionId(txCtx.getTransactionId()); final Graph graph = graphManager.getTraversalSource(traversalSourceName).getGraph(); txCtx.submit(new FutureTask<>(() -> { - graph.tx().open(); + graph.tx().begin(); return null; })).get(5000, TimeUnit.MILLISECONDS); // Not an option for now, but 5s should be plenty. } catch (IllegalStateException ise) { diff --git a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/transaction/UnmanagedTransaction.java b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/transaction/UnmanagedTransaction.java index 3dfc394204..d08c20a47e 100644 --- a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/transaction/UnmanagedTransaction.java +++ b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/transaction/UnmanagedTransaction.java @@ -128,7 +128,7 @@ public class UnmanagedTransaction { */ public void open() { try { - graph.tx().open(); + graph.tx().begin(); touch(); logger.debug("Transaction {} opened", transactionId); } catch (Exception e) { diff --git a/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinDriverTransactionIntegrateTest.java b/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinDriverTransactionIntegrateTest.java index 69f387b17e..51820b98cc 100644 --- a/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinDriverTransactionIntegrateTest.java +++ b/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinDriverTransactionIntegrateTest.java @@ -197,16 +197,22 @@ public class GremlinDriverTransactionIntegrateTest extends AbstractGremlinServer } @Test - public void shouldThrowOnDoubleBegin() throws Exception { + public void shouldBeIdempotentOnDoubleBegin() throws Exception { final RemoteTransaction tx = cluster.transact(GTX); tx.begin(); + final String txId = tx.getTransactionId(); - try { - tx.begin(); - fail("Expected IllegalStateException on second begin()"); - } catch (IllegalStateException ex) { - assertThat(ex.getMessage(), containsString("Transaction already started")); - } + // begin() while already open is idempotent: it does not throw, does not start a new server-side + // transaction (the transactionId is unchanged), and returns a usable source bound to the same tx + final GraphTraversalSource gtx = tx.begin(); + assertTrue(tx.isOpen()); + assertEquals(txId, tx.getTransactionId()); + + // the source from the second begin() works within the same transaction + gtx.addV("person").property("name", "double_begin").iterate(); + assertEquals(1L, (long) gtx.V().has("name", "double_begin").count().next()); + + tx.rollback(); } @Test @@ -276,6 +282,18 @@ public class GremlinDriverTransactionIntegrateTest extends AbstractGremlinServer assertEquals(1L, tx2.submit("g.V().hasLabel('person').count()").one().getLong()); } + @Test + public void shouldBeIdempotentOnDoubleClose() throws Exception { + final RemoteTransaction tx = cluster.transact(GTX); + tx.begin(); + tx.close(); + assertFalse(tx.isOpen()); + + // close() is idempotent: closing an already-closed transaction is a safe no-op (no exception) + tx.close(); + assertFalse(tx.isOpen()); + } + @Test public void shouldRollbackOpenTransactionsOnClusterClose() throws Exception { final RemoteTransaction tx1 = cluster.transact(GTX); @@ -578,12 +596,13 @@ public class GremlinDriverTransactionIntegrateTest extends AbstractGremlinServer assertFalse(tx.isOpen()); assertNull(tx.getTransactionId()); - // second begin should fail — state moved to CLOSED, not back to NOT_STARTED + // second begin should fail — state moved to CLOSED, not back to NOT_STARTED. A remote transaction is + // single-use, so begin() on a closed transaction throws rather than reusing it. try { tx.begin(); fail("Expected IllegalStateException on begin after failed begin"); } catch (IllegalStateException ex) { - assertThat(ex.getMessage(), containsString("Transaction already started")); + assertThat(ex.getMessage(), containsString("Transaction is closed and cannot be reused")); } } diff --git a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/CoreTraversalTest.java b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/CoreTraversalTest.java index 598b37489c..831000c1b0 100644 --- a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/CoreTraversalTest.java +++ b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/CoreTraversalTest.java @@ -217,12 +217,12 @@ public class CoreTraversalTest extends AbstractGremlinProcessTest { g.tx().onReadWrite(Transaction.READ_WRITE_BEHAVIOR.MANUAL); // close down the current transaction and fire up a fresh one - g.tx().open(); + g.tx().begin(); final Traversal t = g.V().has("name", "marko"); g.tx().rollback(); // the traversal should still work since there are auto transactions - g.tx().open(); + g.tx().begin(); assertEquals(1, IteratorUtils.count(t)); g.tx().rollback(); } diff --git a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/structure/TransactionTest.java b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/structure/TransactionTest.java index bd6fc31104..5f75a62e63 100644 --- a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/structure/TransactionTest.java +++ b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/structure/TransactionTest.java @@ -51,7 +51,6 @@ import static org.junit.Assert.fail; * @author Stephen Mallette (http://stephen.genoprime.com) */ @ExceptionCoverage(exceptionClass = Transaction.Exceptions.class, methods = { - "transactionAlreadyOpen", "threadedTransactionsNotSupported", "openTransactionsOnClose", "transactionMustBeOpenToReadWrite", @@ -62,16 +61,17 @@ public class TransactionTest extends AbstractGremlinTest { @Test @FeatureRequirement(featureClass = Graph.Features.GraphFeatures.class, feature = FEATURE_TRANSACTIONS) - public void shouldHaveExceptionConsistencyWhenTransactionAlreadyOpen() { + public void shouldBeIdempotentWhenTransactionAlreadyOpen() { + // begin() is idempotent: calling it when a transaction is already open does not start a new + // transaction and does not throw - it returns a traversal source bound to the open transaction. if (!g.tx().isOpen()) - g.tx().open(); + g.tx().begin(); - try { - g.tx().open(); - fail("An exception should be thrown when a transaction is opened twice"); - } catch (Exception ex) { - validateException(Transaction.Exceptions.transactionAlreadyOpen(), ex); - } + assertThat(g.tx().isOpen(), is(true)); + g.tx().begin(); + assertThat(g.tx().isOpen(), is(true)); + + g.tx().rollback(); } @Test @@ -80,7 +80,7 @@ public class TransactionTest extends AbstractGremlinTest { g.tx().onClose(Transaction.CLOSE_BEHAVIOR.MANUAL); if (!g.tx().isOpen()) - g.tx().open(); + g.tx().begin(); try { graph.tx().close(); @@ -151,6 +151,20 @@ public class TransactionTest extends AbstractGremlinTest { g.tx().rollback(); } + @Test + @FeatureRequirement(featureClass = Graph.Features.GraphFeatures.class, feature = FEATURE_TRANSACTIONS) + public void shouldBeIdempotentWhenClosingAnAlreadyClosedTransaction() { + // close() is idempotent: closing a transaction that is not open is a safe no-op (no exception). + if (g.tx().isOpen()) + g.tx().rollback(); + + assertThat(g.tx().isOpen(), is(false)); + // not expecting any exceptions here - a double/extra close must be a no-op + g.tx().close(); + g.tx().close(); + assertThat(g.tx().isOpen(), is(false)); + } + @Test @FeatureRequirement(featureClass = Graph.Features.GraphFeatures.class, feature = FEATURE_TRANSACTIONS) public void shouldHaveExceptionConsistencyWhenOnCloseToNull() { @@ -762,15 +776,15 @@ public class TransactionTest extends AbstractGremlinTest { @FeatureRequirement(featureClass = Graph.Features.GraphFeatures.class, feature = Graph.Features.GraphFeatures.FEATURE_TRANSACTIONS) public void shouldAllowReferenceOfVertexOutsideOfOriginalTransactionalContextManual() { g.tx().onReadWrite(Transaction.READ_WRITE_BEHAVIOR.MANUAL); - g.tx().open(); + g.tx().begin(); final Vertex v1 = graph.addVertex("name", "stephen"); g.tx().commit(); - g.tx().open(); + g.tx().begin(); assertEquals("stephen", v1.value("name")); g.tx().rollback(); - g.tx().open(); + g.tx().begin(); assertEquals("stephen", v1.value("name")); g.tx().close(); } @@ -780,16 +794,16 @@ public class TransactionTest extends AbstractGremlinTest { @FeatureRequirement(featureClass = Graph.Features.GraphFeatures.class, feature = Graph.Features.GraphFeatures.FEATURE_TRANSACTIONS) public void shouldAllowReferenceOfEdgeOutsideOfOriginalTransactionalContextManual() { g.tx().onReadWrite(Transaction.READ_WRITE_BEHAVIOR.MANUAL); - g.tx().open(); + g.tx().begin(); final Vertex v1 = graph.addVertex(); final Edge e = v1.addEdge("self", v1, "weight", 0.5d); g.tx().commit(); - g.tx().open(); + g.tx().begin(); assertEquals(0.5d, e.value("weight"), 0.00001d); g.tx().rollback(); - g.tx().open(); + g.tx().begin(); assertEquals(0.5d, e.value("weight"), 0.00001d); g.tx().close(); } @@ -827,12 +841,12 @@ public class TransactionTest extends AbstractGremlinTest { @FeatureRequirement(featureClass = Graph.Features.GraphFeatures.class, feature = Graph.Features.GraphFeatures.FEATURE_TRANSACTIONS) public void shouldAllowReferenceOfVertexIdOutsideOfOriginalThreadManual() throws Exception { g.tx().onReadWrite(Transaction.READ_WRITE_BEHAVIOR.MANUAL); - g.tx().open(); + g.tx().begin(); final Vertex v1 = graph.addVertex("name", "stephen"); final AtomicReference<Object> id = new AtomicReference<>(); final Thread t = new Thread(() -> { - g.tx().open(); + g.tx().begin(); id.set(v1.id()); }); @@ -849,13 +863,13 @@ public class TransactionTest extends AbstractGremlinTest { @FeatureRequirement(featureClass = Graph.Features.GraphFeatures.class, feature = Graph.Features.GraphFeatures.FEATURE_TRANSACTIONS) public void shouldAllowReferenceOfEdgeIdOutsideOfOriginalThreadManual() throws Exception { g.tx().onReadWrite(Transaction.READ_WRITE_BEHAVIOR.MANUAL); - g.tx().open(); + g.tx().begin(); final Vertex v1 = graph.addVertex(); final Edge e = v1.addEdge("self", v1, "weight", 0.5d); final AtomicReference<Object> id = new AtomicReference<>(); final Thread t = new Thread(() -> { - g.tx().open(); + g.tx().begin(); id.set(e.id()); }); diff --git a/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerTransaction.java b/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerTransaction.java index 4cf8b50fca..488788597c 100644 --- a/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerTransaction.java +++ b/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerTransaction.java @@ -18,7 +18,6 @@ */ package org.apache.tinkerpop.gremlin.tinkergraph.structure; -import org.apache.tinkerpop.gremlin.process.traversal.TraversalSource; import org.apache.tinkerpop.gremlin.structure.Transaction; import org.apache.tinkerpop.gremlin.structure.util.AbstractThreadLocalTransaction; import org.apache.tinkerpop.gremlin.structure.util.TransactionException; @@ -81,12 +80,6 @@ final class TinkerTransaction extends AbstractThreadLocalTransaction { return txNumber.get() != NOT_STARTED; } - @Override - public <T extends TraversalSource> T begin() { - doOpen(); - return super.begin(); - } - @Override protected void doOpen() { txNumber.set(openedTx.getAndIncrement()); diff --git a/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerTransactionGraphTest.java b/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerTransactionGraphTest.java index 5df8de254f..34340f5262 100644 --- a/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerTransactionGraphTest.java +++ b/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerTransactionGraphTest.java @@ -1415,6 +1415,28 @@ public class TinkerTransactionGraphTest { assertEquals(2L, (long) gtx2.V().count().next()); } + @Test + public void shouldBeIdempotentAndNonLossyWhenBeginCalledWhileOpen() { + final TinkerTransactionGraph g = TinkerTransactionGraph.open(); + final GraphTraversalSource gtx = g.tx().begin(); + + // stage an uncommitted change in the open transaction + gtx.addV().iterate(); + assertTrue(gtx.tx().isOpen()); + assertEquals(1L, (long) gtx.V().count().next()); + + // calling begin() again while already open must be idempotent and non-lossy: it must NOT + // start a new transaction or discard the in-flight one, so the staged change still exists + final GraphTraversalSource gtx2 = g.tx().begin(); + assertTrue(gtx.tx().isOpen()); + assertEquals(1L, (long) gtx2.V().count().next()); + + // the change is part of one continuous transaction - committing once persists exactly it + gtx.tx().commit(); + final GraphTraversalSource gtx3 = g.tx().begin(); + assertEquals(1L, (long) gtx3.V().count().next()); + } + @Test public void shouldHandleAddingPropertyWhenOtherTxAttemptsDeleteThenRollsback() throws InterruptedException { final TinkerTransactionGraph g = TinkerTransactionGraph.open();
