#892 Fix Statement.cancel() causes lockup

(partial backport of #903 from Jaybird 7)

Author: mrotteveel

devdoc/publish.md

@@ -8,7 +8,7 @@ Publishing
 To publish to Maven use
 
 ```
-gradlewclean dist assemble publish -PcredentialsPassphrase=<credentials password>
+gradlew clean dist assemble publish -PcredentialsPassphrase=<credentials password>
 ```
 Where `<credentials password>` is the password used to add the credentials (see
 also below).

src/docs/asciidoc/release_notes.adoc

@@ -37,6 +37,10 @@ For known issues, consult <<known-issues>>.
 
 The following was fixed or changed since Jaybird 6.0.3:
 
+* Fixed: `Statement.cancel()` causes lockup (https://github.com/FirebirdSQL/jaybird/issues/892[#892])
++
+This is a partial backport from Jaybird 7.
+See also <<breaking-changes-internal-api-6-0-4>>.
 * Fixed: Negative buffer size on Firebird 2.5 if information response is 32KiB or greater (https://github.com/FirebirdSQL/jaybird/issues/895[#895])
 +
 The error occurs when preparing or executing statements with a lot of columns, especially select statements with columns that have long names and/or aliases for columns, tables and table owners.
@@ -2093,6 +2097,22 @@ This should only affect maintainers of Jaybird forks, or people using the intern
 ** Constructors now throw `SQLException` if the provided database is null or not attached, or the provided transaction is null or not active.
 Previously, passing in a null transaction would result in a `NullPointerException`.
 
+[#breaking-changes-internal-api-6-0-4]
+==== Breaking changes internal API for 6.0.4
+
+With the introduction of the (partial) transmit lock in the wire protocol to address https://github.com/FirebirdSQL/jaybird/issues/892[#892 Statement.cancel() causes lockup], statement and transaction operations must now hold the transmit lock while sending messages to the server.
+See also https://github.com/FirebirdSQL/jaybird/blob/master/devdoc/jdp/jdp-2026-02-cancellation-thread-safety-backport.adoc[jdp-2026-02: Cancellation thread-safety backport].
+
+Implementations and forks *must* ensure they hold the transmit lock _within_ the connection lock while sending statement or transaction related messages.
+The transmit lock *must* be released before receiving (reading) responses from the server.
+
+We have avoided most of the breaking changes that we made in Jaybird 7, but the following is a minor incompatible change:
+
+* V10Statement
+** `sendExecute(int, RowValue)` should no longer be overridden, instead override `sendExecuteMsg(XdrOutputStream, int, RowValue)`.
++
+If you do continue to override `sendExecute(int, RowValue)`, make sure the transmit lock covers the entire message.
+
 [#breaking-changes-unlikely]
 === Unlikely breaking changes
 

src/main/org/firebirdsql/gds/ng/wire/AbstractFbWireStatement.java

@@ -25,7 +25,6 @@
 import org.firebirdsql.gds.ng.AbstractFbStatement;
 import org.firebirdsql.gds.ng.DeferredResponse;
 import org.firebirdsql.gds.ng.FbDatabase;
-import org.firebirdsql.gds.ng.FbExceptionBuilder;
 import org.firebirdsql.gds.ng.FbStatement;
 import org.firebirdsql.gds.ng.FbTransaction;
 import org.firebirdsql.gds.ng.LockCloseable;
@@ -91,12 +90,16 @@ protected final XdrOutputStream getXdrOut() throws SQLException {
         return getXdrStreamAccess().getXdrOut();
     }
 
-    private XdrStreamAccess getXdrStreamAccess() throws SQLException {
-        if (database != null) {
-            return database.getXdrStreamAccess();
-        } else {
-            throw FbExceptionBuilder.connectionClosed();
-        }
+    /**
+     * @see XdrStreamAccess#withTransmitLock(TransmitAction)
+     * @since 6.0.4
+     */
+    protected final void withTransmitLock(TransmitAction transmitAction) throws IOException, SQLException {
+        getXdrStreamAccess().withTransmitLock(transmitAction);
+    }
+
+    private XdrStreamAccess getXdrStreamAccess() {
+        return database.getXdrStreamAccess();
     }
 
     @Override
@@ -318,11 +321,12 @@ public void run() {
             if (database == null) return;
             try (LockCloseable ignored = database.withLock()) {
                 if (!database.isAttached()) return;
-                XdrOutputStream xdrOut = database.getXdrStreamAccess().getXdrOut();
-                xdrOut.writeInt(WireProtocolConstants.op_free_statement); // p_operation
-                xdrOut.writeInt(handle); // p_sqlfree_statement
-                xdrOut.writeInt(ISCConstants.DSQL_drop); // p_sqlfree_option
-                xdrOut.flush();
+                database.getXdrStreamAccess().withTransmitLock(xdrOut -> {
+                    xdrOut.writeInt(WireProtocolConstants.op_free_statement); // p_operation
+                    xdrOut.writeInt(handle); // p_sqlfree_statement
+                    xdrOut.writeInt(ISCConstants.DSQL_drop); // p_sqlfree_option
+                    xdrOut.flush();
+                });
                 // TODO: This may process deferred actions on the cleaner thread, we may want to change that
                 database.enqueueDeferredAction(CLEANUP_FREE_DEFERRED_ACTION);
             } catch (SQLException | IOException e) {

src/main/org/firebirdsql/gds/ng/wire/AbstractWireOperations.java

@@ -92,6 +92,14 @@ protected final XdrOutputStream getXdrOut() throws SQLException {
         return getXdrStreamAccess().getXdrOut();
     }
 
+    /**
+     * @see XdrStreamAccess#withTransmitLock(TransmitAction)
+     * @since 6.0.4
+     */
+    protected final void withTransmitLock(TransmitAction transmitAction) throws IOException, SQLException {
+        getXdrStreamAccess().withTransmitLock(transmitAction);
+    }
+
     @Override
     public final SQLException readStatusVector() throws SQLException {
         return readStatusVector(getXdrIn());

src/main/org/firebirdsql/gds/ng/wire/TransmitAction.java

@@ -0,0 +1,65 @@
+/*
+ * Public Firebird Java API.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ *    1. Redistributions of source code must retain the above copyright notice,
+ *       this list of conditions and the following disclaimer.
+ *    2. Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in the
+ *       documentation and/or other materials provided with the distribution.
+ *    3. The name of the author may not be used to endorse or promote products
+ *       derived from this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR IMPLIED
+ * WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO
+ * EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
+ * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS;
+ * OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
+ * WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
+ * OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
+ * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+package org.firebirdsql.gds.ng.wire;
+
+import org.firebirdsql.gds.impl.wire.XdrOutputStream;
+import org.jspecify.annotations.NullMarked;
+
+import java.io.IOException;
+import java.sql.SQLException;
+
+/**
+ * Action for transmitting messages to Firebird.
+ * <p>
+ * This action is primarily intended for transmitting data while the transmit lock is held.
+ * </p>
+ *
+ * @see XdrStreamAccess#withTransmitLock(TransmitAction)
+ * @since 6.0.4
+ */
+@FunctionalInterface
+@NullMarked
+public interface TransmitAction {
+
+    /**
+     * Transmits a message (or messages) to Firebird.
+     * <p>
+     * Implementations <strong>should not</strong> obtain (additional) locks, and <strong>must not</strong> attempt to
+     * read (receive) data from the server. Preferably, the only operations done are writes to {@code xdrOut}. In
+     * general, an action should write the whole message. If that is not possible, make sure that the full message is
+     * written while the transmit lock is held.
+     * </p>
+     *
+     * @param xdrOut
+     *         XDR output stream to Firebird server
+     * @throws IOException
+     *         for errors writing into {@code xdrOut}
+     * @throws SQLException
+     *         for connection state errors
+     * @see XdrStreamAccess#withTransmitLock(TransmitAction)
+     */
+    void transmit(XdrOutputStream xdrOut) throws IOException, SQLException;
+
+}

src/main/org/firebirdsql/gds/ng/wire/WireConnection.java

@@ -38,6 +38,7 @@
 import org.firebirdsql.gds.ng.wire.crypt.KnownServerKey;
 import org.firebirdsql.jaybird.props.def.ConnectionProperty;
 import org.firebirdsql.jaybird.util.ByteArrayHelper;
+import org.jspecify.annotations.NonNull;
 
 import javax.net.SocketFactory;
 import java.io.ByteArrayOutputStream;
@@ -59,6 +60,7 @@
 import java.util.Optional;
 import java.util.Properties;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.locks.ReentrantLock;
 
 import static java.lang.System.Logger.Level.DEBUG;
 import static org.firebirdsql.gds.impl.wire.WireProtocolConstants.*;
@@ -98,8 +100,11 @@ public abstract class WireConnection<T extends IAttachProperties<T>, C extends F
     private XdrOutputStream xdrOut;
     private XdrInputStream xdrIn;
     private final XdrStreamAccess streamAccess = new XdrStreamAccess() {
+
+        private final ReentrantLock transmitLock = new ReentrantLock();
+
         @Override
-        public XdrInputStream getXdrIn() throws SQLException {
+        public @NonNull XdrInputStream getXdrIn() throws SQLException {
             if (isConnected() && xdrIn != null) {
                 return xdrIn;
             } else {
@@ -108,13 +113,24 @@ public XdrInputStream getXdrIn() throws SQLException {
         }
 
         @Override
-        public XdrOutputStream getXdrOut() throws SQLException {
+        public @NonNull XdrOutputStream getXdrOut() throws SQLException {
             if (isConnected() && xdrOut != null) {
                 return xdrOut;
             } else {
                 throw FbExceptionBuilder.connectionClosed();
             }
         }
+
+        @Override
+        public void withTransmitLock(@NonNull TransmitAction transmitAction) throws IOException, SQLException {
+            transmitLock.lock();
+            try {
+                transmitAction.transmit(getXdrOut());
+            } finally {
+                transmitLock.unlock();
+            }
+        }
+
     };
 
     /**
@@ -678,7 +694,18 @@ private static String getSystemPropertyPrivileged(final String propertyName) {
      *         If there is no socket, the socket is closed, or for errors writing to the socket.
      */
     public final void writeDirect(byte[] data) throws IOException {
-        xdrOut.writeDirect(data);
+        try {
+            // NOTE: In Jaybird 5 and 6, this is not fully thread-safe, as the transmit lock only covers cancellation
+            // and statement operations; a full solution is available in Jaybird 7 and higher
+            getXdrStreamAccess().withTransmitLock(xdrOut -> {
+                xdrOut.flush();
+                xdrOut.writeDirect(data);
+            });
+        } catch (SQLException e) {
+            // This can happen when withTransmitLock is called on a broken connection; we don't want to change
+            // the signature of this method, so we convert to an IOException
+            throw new IOException(e);
+        }
     }
 
     final List<KnownServerKey.PluginSpecificData> getPluginSpecificData() {

src/main/org/firebirdsql/gds/ng/wire/XdrStreamAccess.java

@@ -1,6 +1,4 @@
 /*
- * $Id$
- *
  * Public Firebird Java API.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -28,7 +26,9 @@
 
 import org.firebirdsql.gds.impl.wire.XdrInputStream;
 import org.firebirdsql.gds.impl.wire.XdrOutputStream;
+import org.jspecify.annotations.NullMarked;
 
+import java.io.IOException;
 import java.sql.SQLException;
 
 /**
@@ -37,25 +37,58 @@
  * @author Mark Rotteveel
  * @since 3.0
  */
+@NullMarked
 public interface XdrStreamAccess {
 
     /**
-     * Gets the XdrInputStream.
+     * Gets the XDR input stream.
      *
-     * @return Instance of XdrInputStream
+     * @return instance of {@link XdrInputStream}
      * @throws SQLException
-     *         If no connection is opened or when exceptions occur
-     *         retrieving the InputStream
+     *         if no connection is opened or when exceptions occur retrieving the InputStream
      */
     XdrInputStream getXdrIn() throws SQLException;
 
     /**
-     * Gets the XdrOutputStream.
+     * Gets the XDR output stream.
      *
-     * @return Instance of XdrOutputStream
+     * @return instance of {@link XdrOutputStream}
      * @throws SQLException
-     *         If no connection is opened or when exceptions occur
-     *         retrieving the OutputStream
+     *         if no connection is opened or when exceptions occur retrieving the OutputStream
+     * @see #withTransmitLock(TransmitAction)
      */
     XdrOutputStream getXdrOut() throws SQLException;
+
+    /**
+     * Runs {@link TransmitAction#transmit(XdrOutputStream)} with {@link #getXdrOut()} on {@code transmitAction} under
+     * the transmit lock.
+     * <p>
+     * For Jaybird 5 and 6, the transmit lock only needs to be used for statement operations and cancellation. See also
+     * <a href="https://github.com/FirebirdSQL/jaybird/blob/master/devdoc/jdp/jdp-2026-02-cancellation-thread-safety-backport.adoc">jdp-2026-02: Cancellation thread-safety backport</a>
+     * </p>
+     * <p>
+     * The transmit lock should only cover sending messages to the server. It should be held for the duration of the
+     * entire message. It <strong>must</strong> be released <em>before</em> reading (receiving) messages from the
+     * server. If possible, do not do anything other than writing to the XDR output stream while holding the lock.
+     * </p>
+     * <p>
+     * Normal operations <strong>must</strong> obtain the lock while holding the connection lock (i.e. the various
+     * {@code withLock()} methods). Out-of-band operations (e.g. cancellation) <strong>must not</strong> take out the
+     * connection lock, otherwise they can't be out-of-band.
+     * </p>
+     * <p>
+     * Note for implementations: the lock used must be reentrant.
+     * </p>
+     *
+     * @param transmitAction
+     *         the transmit action to run under lock
+     * @throws IOException
+     *         for errors writing to the XDR output stream
+     * @throws SQLException
+     *         for other database access errors
+     * @see TransmitAction
+     * @since 6.0.4
+     */
+    void withTransmitLock(TransmitAction transmitAction) throws IOException, SQLException;
+
 }