#905 Fixed incomplete detection of fatal connection errors for deferred actions

Also fixes an NPE when the connection was terminated due to a fatal connection error.

Author: mrotteveel

src/docs/asciidoc/release_notes.adoc

@@ -42,6 +42,12 @@ This fix was backported from Jaybird 6.0.4.
 +
 This is a partial backport from Jaybird 7.
 See also <<breaking-changes-internal-api-5-0-11>>.
+* Fixed: Incomplete detection of fatal connection errors for deferred actions (https://github.com/FirebirdSQL/jaybird/issues/905[#905])
++
+This change also fixes a `NullPointerException` when the connection was terminated due to a fatal connection error.
+A minor breaking change was made in the internal GDS-ng wire API, which should only be relevant to maintainers of Jaybird forks, see <<breaking-changes-internal-api-5-0-11>>.
++
+This fix was backported from Jaybird 6.0.4.
 
 [#jaybird-5-0-10-changelog]
 === Jaybird 5.0.10
@@ -1504,6 +1510,13 @@ We have avoided most of the breaking changes that we made in Jaybird 7, but the
 +
 If you do continue to override `sendExecute(int, RowValue)`, make sure the transmit lock covers the entire message.
 
+Other breaking changes to the internal API in Jaybird 6.0.4 unrelated to the transmit lock:
+
+* `WireConnection`
+** `readNextOperation` now also throws `SQLException` -- if the connection is closed.
+* `FbWireOperations`
+** `readNextOperation` now also throws `SQLException` -- if the connection is closed.
+
 [#breaking-changes-unlikely]
 === Unlikely breaking changes
 

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

@@ -222,9 +222,50 @@ public final Response readResponse(WarningMessageCallback warningCallback) throw
         return wireOperations.readResponse(warningCallback);
     }
 
+    /**
+     * {@inheritDoc}
+     * <p>
+     * The implementation decorates {@code deferredAction} to ensure this database instance is notified of exceptions.
+     * This can result in double notifications if the caller also notifies exceptions and this database instance is
+     * registered as an exception listener for those notifications (this is not harmful).
+     * </p>
+     */
     @Override
     public final void enqueueDeferredAction(DeferredAction deferredAction) {
-        wireOperations.enqueueDeferredAction(deferredAction);
+        wireOperations.enqueueDeferredAction(decorateWithExceptionNotification(deferredAction));
+    }
+
+    private DeferredAction decorateWithExceptionNotification(DeferredAction deferredAction) {
+        return deferredAction instanceof ExceptionNotifyingDeferredAction
+                // Don't decorate again
+                ? deferredAction
+                : new ExceptionNotifyingDeferredAction(deferredAction);
+    }
+
+    private final class ExceptionNotifyingDeferredAction extends DeferredAction.DelegatingDeferredAction {
+
+        ExceptionNotifyingDeferredAction(DeferredAction delegate) {
+            super(delegate);
+        }
+
+        @Override
+        public void onException(Exception exception) {
+            try {
+                super.onException(exception);
+            } finally {
+                exceptionListenerDispatcher.errorOccurred(toReadSQLException(exception));
+            }
+        }
+
+        private SQLException toReadSQLException(Exception exception) {
+            if (exception instanceof SQLException) {
+                return (SQLException) exception;
+            } else if (exception instanceof IOException) {
+                return FbExceptionBuilder.forException(ISCConstants.isc_net_read_err).cause(exception).toSQLException();
+            }
+            return new SQLException(exception);
+        }
+
     }
 
     @Override

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

@@ -179,7 +179,7 @@ public final Response readOperationResponse(int operationCode, WarningMessageCal
     }
 
     @Override
-    public final int readNextOperation() throws IOException {
+    public final int readNextOperation() throws SQLException, IOException {
         try (LockCloseable ignored = withLock()) {
             processDeferredActions();
             return connection.readNextOperation();

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

@@ -31,6 +31,8 @@
 import java.util.function.Consumer;
 import java.util.function.Function;
 
+import static java.util.Objects.requireNonNull;
+
 /**
  * Interface for processing deferred responses from the server.
  * <p>
@@ -110,4 +112,38 @@ public WarningMessageCallback getWarningMessageCallback() {
             }
         };
     }
+
+    /**
+     * Deferred action implementation that delegates to another deferred action.
+     * <p>
+     * This class is intended as a base class for implementations that want to decorate method calls. To decorate it,
+     * subclass this class, and override the method, and ensure you call {@code super.<overridden-method>} in such a
+     * way that it is always called, even if the decoration fails.
+     * </p>
+     * @since 5.0.11
+     */
+    abstract class DelegatingDeferredAction implements DeferredAction {
+
+        private final DeferredAction delegate;
+
+        DelegatingDeferredAction(DeferredAction delegate) {
+            this.delegate = requireNonNull(delegate, "delegate");
+        }
+
+        @Override
+        public void processResponse(Response response) {
+            delegate.processResponse(response);
+        }
+
+        @Override
+        public void onException(Exception exception) {
+            delegate.onException(exception);
+        }
+
+        @Override
+        public WarningMessageCallback getWarningMessageCallback() {
+            return delegate.getWarningMessageCallback();
+        }
+
+    }
 }

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

@@ -75,11 +75,13 @@ public interface FbWireOperations {
      * </p>
      *
      * @return next operation
+     * @throws SQLException
+     *         when the connection is closed
      * @throws java.io.IOException
      *         for errors reading the operation from the connection
      * @since 5.0.7
      */
-    int readNextOperation() throws IOException;
+    int readNextOperation() throws SQLException, IOException;
 
     /**
      * Reads the response from the server when the operation code has already been read.

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

@@ -529,11 +529,14 @@ private FbWireOperations getCryptKeyCallbackWireOperations() {
      * Reads the next operation code. Skips all {@link org.firebirdsql.gds.impl.wire.WireProtocolConstants#op_dummy}
      * codes received.
      *
-     * @return Operation code
+     * @return operation code
+     * @throws SQLException
+     *         when the connection is closed
      * @throws IOException
      *         if an error occurs while reading from the underlying InputStream
      */
-    public final int readNextOperation() throws IOException {
+    public final int readNextOperation() throws SQLException, IOException {
+        XdrInputStream xdrIn = getXdrStreamAccess().getXdrIn();
         int op;
         do {
             op = xdrIn.readInt();

src/test/org/firebirdsql/management/FBMaintenanceManagerTest.java

@@ -22,7 +22,6 @@
 import org.firebirdsql.common.extension.UsesDatabaseExtension;
 import org.firebirdsql.gds.ISCConstants;
 import org.firebirdsql.gds.TransactionParameterBuffer;
-import org.firebirdsql.gds.impl.GDSType;
 import org.firebirdsql.gds.ng.FbDatabase;
 import org.firebirdsql.gds.ng.FbTransaction;
 import org.firebirdsql.jdbc.FBConnection;
@@ -53,8 +52,11 @@
 import static org.firebirdsql.common.matchers.MatcherAssume.assumeThat;
 import static org.firebirdsql.common.matchers.SQLExceptionMatchers.errorCode;
 import static org.firebirdsql.common.matchers.SQLExceptionMatchers.errorCodeEquals;
+import static org.firebirdsql.common.matchers.SQLExceptionMatchers.message;
 import static org.hamcrest.CoreMatchers.not;
+import static org.hamcrest.CoreMatchers.startsWith;
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.anyOf;
 import static org.hamcrest.Matchers.oneOf;
 import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
 import static org.junit.jupiter.api.Assertions.assertEquals;
@@ -202,8 +204,10 @@ void testForcedShutdown(boolean autoCommit) throws Exception {
                 maintenanceManager.shutdownDatabase(MaintenanceManager.SHUTDOWN_FORCE, 0);
 
                 SQLException exception = assertThrows(SQLException.class, () -> stmt.executeQuery(sql));
-                assertThat(exception, errorCode(oneOf(
-                        ISCConstants.isc_shutdown, ISCConstants.isc_att_shutdown, ISCConstants.isc_net_read_err)));
+                assertThat(exception, anyOf(
+                        errorCode(oneOf(ISCConstants.isc_shutdown, ISCConstants.isc_att_shutdown,
+                                ISCConstants.isc_net_read_err)),
+                        message(startsWith("Connection closed"))));
             } finally {
                 closeQuietly(stmt);
             }