TINKERPOP-3252 Replace Transaction.open() with idempotent begin()

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`.

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

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

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 extends Graph> 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 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 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<Transaction> {
         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");

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 extends Graph> G createThreadedTx() {
         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);
     }
 

gremlin-dotnet/src/Gremlin.Net/Driver/Remote/TransactionRemoteConnection.cs

@@ -97,8 +97,8 @@ public async Task<ITraversal<TStart, TEnd>> SubmitAsync<TStart, TEnd>(GremlinLan
         /// <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;
         }
 

gremlin-dotnet/src/Gremlin.Net/Driver/RemoteTransaction.cs

@@ -73,68 +73,82 @@ internal RemoteTransaction(IGremlinClient client, string traversalSource)
 
         /// <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>