From 433be658d515c0aab2a1e910f13d97f0daabf8e4 Mon Sep 17 00:00:00 2001 From: Ken Hu <106191785+kenhuuu@users.noreply.github.com> Date: Thu, 18 Jun 2026 20:13:52 -0700 Subject: [PATCH] TINKERPOP-3252 Replace Transaction.open() with idempotent begin() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 37 +++++-- docs/src/upgrade/release-4.x.x.asciidoc | 15 +++ .../gremlin/structure/Transaction.java | 29 ++---- .../structure/util/AbstractTransaction.java | 27 +++--- .../Remote/TransactionRemoteConnection.cs | 4 +- .../Gremlin.Net/Driver/RemoteTransaction.cs | 96 +++++++++++-------- .../Driver/TransactionTests.cs | 63 +++++++++--- .../driver/remote/HttpRemoteTransaction.java | 65 +++++++------ gremlin-go/driver/error_codes.go | 2 +- .../driver/resources/error-messages/en.json | 2 +- gremlin-go/driver/transaction.go | 58 ++++++----- gremlin-go/driver/transaction_test.go | 61 ++++++++---- .../lib/process/transaction.ts | 57 ++++++----- .../test/integration/transaction-tests.js | 73 +++++++++----- .../gremlin_python/driver/transaction.py | 62 ++++++------ .../integration/driver/test_transaction.py | 61 ++++++++---- .../handler/HttpGremlinEndpointHandler.java | 4 +- .../transaction/UnmanagedTransaction.java | 2 +- ...GremlinDriverTransactionIntegrateTest.java | 55 +++++++---- .../process/traversal/CoreTraversalTest.java | 4 +- .../gremlin/structure/TransactionTest.java | 83 ++++++++-------- .../structure/TinkerTransaction.java | 7 -- .../structure/TinkerTransactionGraphTest.java | 22 +++++ 24 files changed, 557 insertions(+), 334 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index d2e0ad55cfb..dab6261d31f 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -54,6 +54,8 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima ** Added a `proxy(ProxyOptions)` builder that inserts a Netty `HttpProxyHandler` into the pipeline before the SSL handler. ** Removed the `maxResponseContentLength` connection option and its `HttpObjectAggregator` cap; responses are streamed and the new `readTimeout` is the partial mitigation. ** Reconciled the `validationRequest` default: the builder default is now `g.inject(0)` to match the `Settings` default (it was previously `''`). +* 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 b085b4a0848..b3116328ac4 100644 --- a/docs/src/reference/the-traversal.asciidoc +++ b/docs/src/reference/the-traversal.asciidoc @@ -135,9 +135,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] ---- @@ -259,8 +260,28 @@ 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, by default (excluding threaded transactions), 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. Threaded transactions obtained via `createThreadedTx()` differ in that multiple threads +can collaborate on the same transaction, but the lifecycle after commit or rollback is not specified by the API and is +provider-defined. +* *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 @@ -307,8 +328,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() @@ -322,7 +343,7 @@ 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. <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 1558bcd027d..2abc88e1e59 100644 --- a/docs/src/upgrade/release-4.x.x.asciidoc +++ b/docs/src/upgrade/release-4.x.x.asciidoc @@ -295,6 +295,21 @@ examples. See: link:https://issues.apache.org/jira/browse/TINKERPOP-3253[TINKERPOP-3253] +==== `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. Instead it returns a `TraversalSource` bound to the existing transaction. For embedded graphs this +replaces the previous behavior where opening an already-open transaction threw an exception, so review and remove any +code that catches 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 45e6273d85c..b04dea3537d 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 @@ private Symbols() { } //////////////// - /** - * 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 default G createThreadedTx() { } /** - * 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 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 begin(final Class traversalSourceClass); @@ -153,10 +153,6 @@ public static class Exceptions { 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 enum READ_WRITE_BEHAVIOR implements Consumer { AUTO { @Override public void accept(final Transaction transaction) { - if (!transaction.isOpen()) transaction.open(); + if (!transaction.isOpen()) transaction.begin(); } }, @@ -240,11 +236,6 @@ public void accept(final Transaction transaction) { } 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 bd54e083197..c9575ba7710 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 AbstractTransaction(final Graph graph) { } /** - * 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 AbstractTransaction(final Graph graph) { */ protected abstract void doClose(); - /** - * {@inheritDoc} - */ - @Override - public void open() { - if (isOpen()) - throw Transaction.Exceptions.transactionAlreadyOpen(); - else - doOpen(); - } - /** * {@inheritDoc} */ @@ -123,8 +112,20 @@ public G createThreadedTx() { throw Transaction.Exceptions.threadedTransactionsNotSupported(); } + /** + * {@inheritDoc} + *

+ * 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 begin(final Class 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 9fa1e57441d..d63ec834f7d 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 @@ public async Task> SubmitAsync(GremlinLan /// 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 f7f0f93fbc8..7e2ef2f8739 100644 --- a/gremlin-dotnet/src/Gremlin.Net/Driver/RemoteTransaction.cs +++ b/gremlin-dotnet/src/Gremlin.Net/Driver/RemoteTransaction.cs @@ -73,68 +73,82 @@ internal RemoteTransaction(IGremlinClient client, string traversalSource) ///

/// Starts the transaction and returns a transaction-bound . + /// + /// 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. + /// /// /// The token to cancel the operation. /// A bound to this transaction. - /// Thrown if the transaction is already started. + /// Thrown if the transaction has already been closed. public async Task 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 results; + var requestMsg = RequestMessage.Build("g.tx().begin()") + .AddG(_traversalSource) + .Create(); + + await _submitLock.WaitAsync(cancellationToken).ConfigureAwait(false); try { - var resultSet = await _client.SubmitAsync(requestMsg, cancellationToken) - .ConfigureAwait(false); - results = await resultSet.ToListAsync(cancellationToken).ConfigureAwait(false); - } - catch - { - _failed = true; - throw; - } + List results; + try + { + var resultSet = await _client.SubmitAsync(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 resultMap && - resultMap.TryGetValue("transactionId", out var txIdObj) && - txIdObj is string txId && !string.IsNullOrEmpty(txId)) - { - _transactionId = txId; + if (results[0] is Dictionary 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(), new GremlinLang(), - _txConnection); + _txConnection!); } /// diff --git a/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Driver/TransactionTests.cs b/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Driver/TransactionTests.cs index 9e62f09c38b..c6efeb333d4 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 @@ await Assert.ThrowsAsync( } [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(() => 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 @@ public async Task ShouldRollbackOnDisposeByDefault() 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 @@ public async Task ShouldReturnSameTransactionFromGtxTx() } [Fact] - public async Task ShouldThrowOnBeginFromGtxTx() + public async Task ShouldBeIdempotentOnBeginFromGtxTx() { using var client = CreateClient(); await DropGraph(client); @@ -329,9 +357,14 @@ public async Task ShouldThrowOnBeginFromGtxTx() var tx = g.Tx(); var gtx = await tx.BeginAsync(); + var txId = tx.TransactionId; var sameTx = gtx.Tx(); - await Assert.ThrowsAsync(() => 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(); } @@ -585,22 +618,24 @@ public async Task ShouldReturnBodyValueFromEvaluateInTxAsync() } [Fact] - public async Task ShouldThrowWhenOpeningNestedTransactionInsideExecuteInTxAsync() + public async Task ShouldBeIdempotentWhenBeginningInsideExecuteInTxAsync() { using var client = CreateClient(); await DropGraph(client); var connection = new DriverRemoteConnection(client, "gtx"); var g = AnonymousTraversalSource.Traversal().With(connection); - // Opening a SECOND transaction from inside the body must throw. The closure body's - // own commit will then fail because the body threw, surfacing the nesting error. - await Assert.ThrowsAsync(() => - g.ExecuteInTxAsync(async gtx => - { - // gtx.Tx() legitimately returns the SAME transaction (it must not throw); - // calling BeginAsync() on it opens a second transaction and must throw. - await gtx.Tx().BeginAsync(); - })); + // BeginAsync() from inside the body, on the same already-open transaction returned by + // gtx.Tx(), is idempotent: it does not throw and does not start a new server-side + // transaction (the transaction id is unchanged). + await g.ExecuteInTxAsync(async gtx => + { + var tx = gtx.Tx(); + var txId = tx.TransactionId; + var gtx2 = await tx.BeginAsync(); + Assert.NotNull(gtx2); + Assert.Equal(txId, gtx2.Tx().TransactionId); + }); } [Fact] 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 2b7a262d6db..a245e36b035 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 HttpRemoteTransaction(final Client.PinnedClient pinnedClient, final Strin 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. *

- * 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 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 begin(final Class 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 void readWrite() { @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 2d3c6b89170..9e727c04606 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 fc33202bf5f..0c8fa6c4bd1 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 e57177a65cc..cbbdb3f2af0 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 9c9cc17674d..c36efadc3f7 100644 --- a/gremlin-go/driver/transaction_test.go +++ b/gremlin-go/driver/transaction_test.go @@ -188,10 +188,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) { @@ -410,7 +416,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() @@ -421,11 +427,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() } @@ -484,6 +494,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() @@ -718,9 +746,9 @@ func TestTransactionEvaluateInTxReturnsValue(t *testing.T) { // Opening a SECOND transaction from inside the body must error. gtx.Tx() itself // legitimately returns the SAME transaction (it must not error - that is the -// commit path), but a nested Begin() on it is rejected by the existing -// double-begin guard (E1101). -func TestTransactionExecuteInTxRejectsNestedBegin(t *testing.T) { +// commit path), and a nested Begin() on it is idempotent: it does not error and +// reuses the same already-open transaction. +func TestTransactionExecuteInTxBeginIsIdempotent(t *testing.T) { client := newTxClient(t) defer client.Close() dropTxGraph(t, client) @@ -733,21 +761,22 @@ func TestTransactionExecuteInTxRejectsNestedBegin(t *testing.T) { // gtx.Tx() returns the bound, already-open transaction (must NOT error). tx := gtx.Tx() assert.True(t, tx.IsOpen()) + txId := tx.TransactionId() // gtx.Tx() called again returns that same transaction handle. assert.Equal(t, tx, gtx.Tx()) - // Opening a second transaction (a nested begin) IS rejected. - _, beginErr := tx.Begin() - assert.NotNil(t, beginErr) - assert.Contains(t, beginErr.Error(), "E1101") + // Begin() on the already-open transaction is idempotent: no error, no new server-side + // transaction (the transaction id is unchanged). + gtx2, beginErr := tx.Begin() + assert.Nil(t, beginErr) + assert.NotNil(t, gtx2) + assert.Equal(t, txId, gtx2.Tx().TransactionId()) - // Surface the nested-begin error from the body so the wrapper rolls back. - return beginErr + return nil }) - assert.NotNil(t, err) - assert.Contains(t, err.Error(), "E1101") + assert.Nil(t, err) } // Verifies that a failure of the wrapper's automatic Commit() is surfaced from diff --git a/gremlin-js/gremlin-javascript/lib/process/transaction.ts b/gremlin-js/gremlin-javascript/lib/process/transaction.ts index 0296edfb47a..57b77022b27 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} */ async begin(): Promise { - 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 01a95367dfd..cce42182c39 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(); @@ -480,19 +504,20 @@ describe('Transaction', function () { await connection.close(); }); - it('should reject opening a nested transaction in the body', async function () { + it('should be idempotent when beginning inside the body', async function () { const connection = getConnection('gtx'); const g = anon.traversal().withRemote(connection); - await assert.rejects( - () => - g.executeInTx(async (gtx) => { - // gtx.tx() legitimately returns the SAME transaction; calling begin() - // on it opens a second transaction and trips the double-begin guard. - await gtx.tx().begin(); - }), - /Transaction already started/ - ); + await g.executeInTx(async (gtx) => { + // gtx.tx() legitimately returns the SAME (already-open) transaction; calling begin() + // on it is idempotent - it does not throw and does not start a new server-side + // transaction (the transaction id is unchanged). + const tx = gtx.tx(); + const txId = tx.transactionId; + const gtx2 = await tx.begin(); + assert.ok(gtx2); + assert.strictEqual(gtx2.tx().transactionId, txId); + }); await connection.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 fbf1170cc59..857f1a2e3d6 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 @@ def begin(self): 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 d7f2da1111b..6456a1e8645 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 @@ def test_should_throw_on_submit_after_rollback(self, client): 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 @@ def test_should_rollback_on_close_by_default(self, client): 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 @@ def test_should_return_same_transaction_from_gtx_tx(self, client): 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 @@ def test_should_not_allow_begin_after_commit(self, client): 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 @@ def test_should_not_allow_begin_after_rollback(self, client): 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() @@ -449,19 +475,20 @@ def test_execute_in_tx_returns_body_value(self, remote_connection): count = g.execute_in_tx(lambda gtx: gtx.V().count().next()) assert count == 2 - def test_execute_in_tx_rejects_nested_transaction(self, remote_connection): - # Opening a SECOND transaction inside the body must raise. gtx.tx() - # itself legitimately returns the same transaction (so we do NOT assert - # it raises); calling begin() on it does raise. + def test_execute_in_tx_begin_is_idempotent(self, remote_connection): + # gtx.tx() returns the same (already-open) transaction; calling begin() on it inside the + # body is idempotent - it does not raise and does not start a new server-side transaction, + # returning a source bound to the same transaction (unchanged transaction id). g = traversal().with_(remote_connection) def body(gtx): - # gtx.tx() returns the same (already-open) transaction; begin() - # on an already-open transaction is the double-begin guard. - gtx.tx().begin() + tx = gtx.tx() + tx_id = tx.transaction_id + gtx2 = tx.begin() + assert gtx2 is not None + assert gtx2.tx().transaction_id == tx_id - with pytest.raises(Exception, match="Transaction already started"): - g.execute_in_tx(body) + g.execute_in_tx(body) def test_execute_in_tx_propagates_commit_failure(self, remote_connection): # To drive a deterministic, no-mock commit failure, the body succeeds but 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 11939008a8d..ae540582b3d 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 @@ -496,7 +496,7 @@ public void exceptionCaught(final ChannelHandlerContext ctx, final Throwable cau } /** - * 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); @@ -507,7 +507,7 @@ private void doBegin(final Context ctx) throws Exception { 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 3dfc3942046..d08c20a47e3 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 void touch() { */ 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 0a67f9cf71a..855acdde2c6 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 @@ -198,16 +198,22 @@ public void shouldThrowOnSubmitAfterRollback() throws Exception { } @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 @@ -277,6 +283,18 @@ public void shouldCommitOnCloseWhenConfigured() throws Exception { 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); @@ -579,12 +597,13 @@ public void shouldCleanUpOnBeginFailure() throws Exception { 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")); } } @@ -773,7 +792,7 @@ public void shouldReturnBodyValueFromTxClosureCall() throws Exception { } @Test - public void shouldRejectOpeningSecondTransactionInsideTxClosureBody() throws Exception { + public void shouldBeIdempotentWhenBeginningInsideTxClosureBody() throws Exception { final GraphTraversalSource g = traversal().with(DriverRemoteConnection.using(cluster, GTX)); g.executeInTx(gtx -> { @@ -781,17 +800,13 @@ public void shouldRejectOpeningSecondTransactionInsideTxClosureBody() throws Exc // standard way to commit/rollback when holding a transactional source. final Transaction nested = gtx.tx(); assertNotNull(nested); + final String txId = ((RemoteTransaction) nested).getTransactionId(); - // opening a SECOND transaction from within an already-open one must raise. The remote - // HttpRemoteTransaction.begin() guards against double-begin, so the remote nesting test asserts on begin(). - try { - nested.begin(); - fail("Opening a second transaction from within an already-open one should raise"); - } catch (Exception ex) { - // expected - the transaction is already started - assertThat(ex, instanceOf(IllegalStateException.class)); - assertThat(ex.getMessage(), containsString("Transaction already started")); - } + // begin() from within an already-open transaction is idempotent: it does not throw and does not start a + // new server-side transaction (the transactionId is unchanged), returning a source bound to the same tx. + final GraphTraversalSource gtx2 = nested.begin(); + assertNotNull(gtx2); + assertEquals(txId, ((RemoteTransaction) gtx2.tx()).getTransactionId()); gtx.addV("person").iterate(); }); 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 598b37489c5..831000c1b05 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 void shouldTraverseIfManualTxEnabledAndOriginalTxIsClosed() { 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 4efc21576be..0fd1191145e 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 @@ -23,6 +23,7 @@ import org.apache.tinkerpop.gremlin.FeatureRequirement; import org.apache.tinkerpop.gremlin.FeatureRequirementSet; import org.apache.commons.configuration2.Configuration; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; import org.apache.tinkerpop.gremlin.structure.util.CloseableIterator; import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils; import org.junit.Test; @@ -52,7 +53,6 @@ * @author Stephen Mallette (http://stephen.genoprime.com) */ @ExceptionCoverage(exceptionClass = Transaction.Exceptions.class, methods = { - "transactionAlreadyOpen", "threadedTransactionsNotSupported", "openTransactionsOnClose", "transactionMustBeOpenToReadWrite", @@ -63,16 +63,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 @@ -81,7 +82,7 @@ public void shouldHaveExceptionConsistencyWhenTransactionOpenOnClose() { g.tx().onClose(Transaction.CLOSE_BEHAVIOR.MANUAL); if (!g.tx().isOpen()) - g.tx().open(); + g.tx().begin(); try { graph.tx().close(); @@ -152,6 +153,20 @@ public void shouldAllowJustRollbackOnlyWithAutoTransaction() { 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() { @@ -763,15 +778,15 @@ public void run() { @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(); } @@ -781,16 +796,16 @@ public void shouldAllowReferenceOfVertexOutsideOfOriginalTransactionalContextMan @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(); } @@ -828,12 +843,12 @@ public void shouldAllowReferenceOfEdgeOutsideOfOriginalTransactionalContextAuto( @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 id = new AtomicReference<>(); final Thread t = new Thread(() -> { - g.tx().open(); + g.tx().begin(); id.set(v1.id()); }); @@ -850,13 +865,13 @@ public void shouldAllowReferenceOfVertexIdOutsideOfOriginalThreadManual() throws @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 id = new AtomicReference<>(); final Thread t = new Thread(() -> { - g.tx().open(); + g.tx().begin(); id.set(e.id()); }); @@ -1043,29 +1058,15 @@ public void shouldReturnBodyValueFromTxClosureCall() { @Test @FeatureRequirement(featureClass = Graph.Features.VertexFeatures.class, feature = Graph.Features.VertexFeatures.FEATURE_ADD_VERTICES) @FeatureRequirement(featureClass = Graph.Features.GraphFeatures.class, feature = FEATURE_TRANSACTIONS) - public void shouldRejectOpeningSecondTransactionInsideTxClosureBody() { + public void shouldAllowIdempotentBeginInsideTxClosureBody() { g.executeInTx(gtx -> { - // gtx.tx() itself is legitimate and returns the (same) transaction - it must NOT throw, as it is the - // standard way to commit/rollback when holding a transactional source. - final Transaction nested = gtx.tx(); - assertNotNull(nested); - - // opening a SECOND transaction from within an already-open one must raise. On the embedded impl - // (TinkerTransaction) begin() calls doOpen() unconditionally with no double-open guard, so the guard lives - // in AbstractTransaction.open() (transactionAlreadyOpen()) - hence the embedded nesting test asserts on - // open(), not begin(). - try { - nested.open(); - fail("Opening a second transaction from within an already-open one should raise"); - } catch (Exception ex) { - // expected - a transaction is already open - assertThat(ex, instanceOf(IllegalStateException.class)); - } + // calling begin() again inside the closure is idempotent: the transaction is already open so + // begin() returns a source bound to the same underlying transaction. + final GraphTraversalSource gtx2 = gtx.tx().begin(); + assertNotNull(gtx2); - gtx.addV("person").iterate(); + // both sources reference the same Transaction + assertEquals(gtx.tx(), gtx2.tx()); }); - - // the outer transaction still committed normally - assertEquals(1L, g.V().hasLabel("person").count().next().longValue()); } } 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 4cf8b50fca5..488788597c8 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 @@ public boolean isOpen() { return txNumber.get() != NOT_STARTED; } - @Override - public 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 5df8de254fd..34340f5262a 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 void shouldReopenClosedTransaction() { 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();