You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by ke...@apache.org on 2026/06/19 03:16:26 UTC
(tinkerpop) 01/01: TINKERPOP-3252 Replace Transaction.open() with idempotent begin()
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 <10...@users.noreply.github.com>
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();