Fixes erroneous closing behavior in JtaConnection.java (helidon-io#6321)

* Fixes erroneous closing behavior in JtaConnection.java

Signed-off-by: Laird Nelson <laird.nelson@oracle.com>

Author: ljnelson

integrations/jdbc/jdbc/src/main/java/io/helidon/integrations/jdbc/ConditionallyCloseableConnection.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2021, 2022 Oracle and/or its affiliates.
+ * Copyright (c) 2021, 2023 Oracle and/or its affiliates.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -94,28 +94,42 @@ public class ConditionallyCloseableConnection extends DelegatingConnection {
     private SQLBooleanSupplier isClosedFunction;
 
     /**
-     * Whether or not the {@link #close()} method will actually close this {@link DelegatingConnection}.
+     * The internal state of this {@link ConditionallyCloseableConnection}.
      *
-     * <p>This field is set based on the value of the {@code strictClosedChecking} argument supplied to the {@link
-     * #ConditionallyCloseableConnection(Connection, boolean, boolean)} constructor. It may end up deliberately doing
-     * nothing.</p>
+     * <p>This field is never {@code null}.</p>
+     *
+     * <!--
+     * digraph ConditionallyCloseableConnection {
+     *
+     *   CLOSEABLE -> CLOSED [label="close()"];
+     *   CLOSEABLE -> NOT_CLOSEABLE [label="setCloseable(false)"];
+     *   CLOSEABLE -> CLOSEABLE;
+     *
+     *   NOT_CLOSEABLE -> CLOSE_PENDING [label="close()"];
+     *   NOT_CLOSEABLE -> NOT_CLOSEABLE [label="setCloseable(false), isCloseable(), isClosed()"];
+     *   NOT_CLOSEABLE -> CLOSEABLE [label="setCloseable(true)"];
+     *
+     *   CLOSE_PENDING -> CLOSE_PENDING [label="close(), setCloseable(false), isCloseable(), isClosed()"];
+     *   CLOSE_PENDING -> CLOSEABLE [label="setCloseable(true)"];
+     *
+     *   CLOSED -> CLOSED [label="close(), isCloseable(), isClosed()"];
+     *
+     * }
+     * -->
+     *
+     * @see #isClosed()
      *
      * @see #isCloseable()
      *
      * @see #setCloseable(boolean)
      *
-     * @see #ConditionallyCloseableConnection(Connection, boolean, boolean)
-     */
-    private volatile boolean closeable;
-
-    /**
-     * Whether or not a {@link #close()} request has been issued from another thread.
+     * @see #close()
      *
      * @see #isClosePending()
      *
-     * @see #isClosed()
+     * @see #ConditionallyCloseableConnection(Connection, boolean, boolean)
      */
-    private volatile boolean closePending;
+    private volatile State state;
 
 
     /*
@@ -205,12 +219,12 @@ public ConditionallyCloseableConnection(Connection delegate,
         super(delegate);
         if (strictClosedChecking) {
             this.closedChecker = this::failWhenClosed;
-            this.isClosedFunction = () -> this.isClosePending() || super.isClosed();
+            this.isClosedFunction = this::strictIsClosed;
         } else {
             this.closedChecker = ConditionallyCloseableConnection::doNothing;
             this.isClosedFunction = super::isClosed;
         }
-        this.closeable = closeable;
+        this.state = closeable ? State.CLOSEABLE : State.NOT_CLOSEABLE;
     }
 
 
@@ -252,15 +266,29 @@ public ConditionallyCloseableConnection(Connection delegate,
     @Override // DelegatingConnection
     public void close() throws SQLException {
         // this.checkOpen(); // Deliberately omitted per spec.
-        if (this.isCloseable()) {
+        switch (this.state) {
+        case CLOSEABLE:
             try {
                 super.close();
             } finally {
-                this.closePending = false;
+                this.state = State.CLOSED;
                 this.onClose();
             }
-        } else {
-            this.closePending = true;
+            break;
+        case NOT_CLOSEABLE:
+            this.state = State.CLOSE_PENDING;
+            break;
+        case CLOSE_PENDING:
+            break;
+        case CLOSED:
+            try {
+                super.close();
+            } finally {
+                this.onClose();
+            }
+            break;
+        default:
+            throw new AssertionError();
         }
     }
 
@@ -329,7 +357,21 @@ protected void onClose() throws SQLException {
      */
     public boolean isCloseable() throws SQLException {
         // this.checkOpen(); // Deliberately omitted.
-        return this.closeable && !this.isClosed();
+        switch (this.state) {
+        case CLOSEABLE:
+            return !this.isClosed(); // reduces to !super.isClosed() which reduces to !this.delegate().isClosed()
+        case NOT_CLOSEABLE:
+            assert !this.isClosed(); // reduces to !super.isClosed() which reduces to !this.delegate().isClosed()
+            return false;
+        case CLOSE_PENDING:
+            // (Can't assert about isClosed() because its behavior depends on strictClosedChecking constructor parameter.)
+            return false;
+        case CLOSED:
+            assert this.isClosed(); // reduces to super.isClosed() which reduces to this.delegate().isClosed()
+            return false;
+        default:
+            throw new AssertionError();
+        }
     }
 
     /**
@@ -364,9 +406,22 @@ public boolean isCloseable() throws SQLException {
      */
     public void setCloseable(boolean closeable) {
         // this.checkOpen(); // Deliberately omitted.
-        this.closeable = closeable;
-        if (closeable) {
-            this.closePending = false;
+        switch (this.state) {
+        case CLOSEABLE:
+            if (!closeable) {
+                this.state = State.NOT_CLOSEABLE;
+            }
+            break;
+        case NOT_CLOSEABLE:
+        case CLOSE_PENDING:
+            if (closeable) {
+                this.state = State.CLOSEABLE;
+            }
+            break;
+        case CLOSED:
+            break;
+        default:
+            throw new AssertionError();
         }
     }
 
@@ -393,7 +448,7 @@ public void setCloseable(boolean closeable) {
      */
     public boolean isClosePending() {
         // this.checkOpen(); // Deliberately omitted.
-        return this.closePending;
+        return this.state == State.CLOSE_PENDING;
     }
 
     @Override // DelegatingConnection
@@ -481,6 +536,11 @@ public boolean isClosed() throws SQLException {
         return this.isClosedFunction.getAsBoolean();
     }
 
+    // (Invoked by method reference only.)
+    private boolean strictIsClosed() throws SQLException {
+        return this.isClosePending() || super.isClosed();
+    }
+
     @Override // DelegatingConnection
     public DatabaseMetaData getMetaData() throws SQLException {
         this.checkOpen();
@@ -866,4 +926,50 @@ private static void doNothing() {
 
     }
 
+
+    /*
+     * Inner and nested classes.
+     */
+
+
+    /**
+     * A state that a {@link ConditionallyCloseableConnection} can have.
+     */
+    private enum State {
+
+        /**
+         * A {@link State} indicating that an invocation of a {@link ConditionallyCloseableConnection}'s {@link
+         * ConditionallyCloseableConnection#close() close()} method will close {@linkplain
+         * ConditionallyCloseableConnection#delegate() its underlying delegate}.
+         */
+        CLOSEABLE,
+
+        /**
+         * A {@link State} indicating that an invocation of a {@link ConditionallyCloseableConnection}'s {@link
+         * ConditionallyCloseableConnection#close() close()} method will place it into the {@link #CLOSE_PENDING} state.
+         *
+         * @see ConditionallyCloseableConnection#setCloseable(boolean)
+         */
+        NOT_CLOSEABLE,
+
+        /**
+         * A {@link State} indicating that an invocation of a {@link ConditionallyCloseableConnection}'s {@link
+         * ConditionallyCloseableConnection#close() close()} method has placed it into this state, and actual closing of
+         * {@linkplain ConditionallyCloseableConnection#delegate() its underlying delegate} will need to be arranged.
+         *
+         * @see ConditionallyCloseableConnection#setCloseable(boolean)
+         */
+        CLOSE_PENDING,
+
+        /**
+         * A {@link State} indicating that an invocation of a {@link ConditionallyCloseableConnection}'s {@link
+         * ConditionallyCloseableConnection#close() close()} method has placed it into this state, and that {@linkplain
+         * ConditionallyCloseableConnection#delegate() its underlying delegate} has also been closed.
+         *
+         * <p>This is a terminal state.</p>
+         */
+        CLOSED;
+
+    }
+
 }

integrations/jta/jdbc/src/main/java/io/helidon/integrations/jta/jdbc/JtaConnection.java

@@ -1049,7 +1049,7 @@ private void enlist() throws SQLException {
             throw new SQLTransientException("xaResourceSupplier.get() == null");
         }
 
-        Enlistment enlistment = new Enlistment(t, xar);
+        Enlistment enlistment = new Enlistment(Thread.currentThread().getId(), t, xar);
         if (!ENLISTMENT.compareAndSet(this, null, enlistment)) { // atomic volatile write
             // Setting this.enlistment could conceivably fail if another thread already enlisted this JtaConnection.
             // That would be bad.
@@ -1124,27 +1124,18 @@ private Connection connectionFunction(Xid xid) {
     private void transactionCompleted(int commitedOrRolledBack) {
         this.enlistment = null; // volatile write
         try {
-            if (!super.isClosed()) {
-                // Did someone call close() while we were enlisted?  That's permitted by section 4.2 of the JTA
-                // specification, but it shouldn't actually close the connection because the prepare/commit cycle
-                // wouldn't have started.
-                boolean closeWasPending = this.isClosePending(); // volatile read
-
-                // If they did, then we must be non-closeable. If they didn't, we could be either closeable or not
-                // closeable.
-                assert !closeWasPending || !this.isCloseable();
-
-                // Now the global transaction is over, so blindly set our closeable status to true. (It may already be
-                // true.)  This resets the closePending status, per spec.
-
-                this.setCloseable(true);
-                assert this.isCloseable();
+            boolean closeWasPending = this.isClosePending();
+            this.setCloseable(true);
+            assert this.isCloseable();
+            assert !this.isClosePending();
+            if (closeWasPending) {
+                assert !this.isClosed();
+                assert !this.delegate().isClosed();
+                this.close();
+                assert this.isClosed();
+                assert this.delegate().isClosed();
+                assert !this.isCloseable();
                 assert !this.isClosePending();
-
-                // If there WAS a close() attempt, now it CAN work, so let it work.
-                if (closeWasPending) {
-                    this.close();
-                }
             }
         } catch (SQLException e) {
             // (Synchronization implementations can throw only unchecked exceptions.)
@@ -1200,10 +1191,6 @@ default void beforeCompletion() {
 
     private static final record Enlistment(long threadId, Transaction transaction, XAResource xaResource) {
 
-        private Enlistment(Transaction transaction, XAResource xaResource) {
-            this(Thread.currentThread().getId(), transaction, xaResource);
-        }
-
         private Enlistment {
             Objects.requireNonNull(transaction, "transaction");
             Objects.requireNonNull(xaResource, "xaResource");

integrations/jta/jdbc/src/test/java/io/helidon/integrations/jta/jdbc/TestJtaConnection.java

@@ -332,7 +332,6 @@ final void testBeginSuspendBeginCommitResumeCommit()
         // The TransactionManager will report that there is no transaction.
         assertThat(tm.getStatus(), is(Status.STATUS_NO_TRANSACTION));
 
-        // assertThat(logicalConnection.enlisted(), is(true)); // we're enlisted in a suspended transaction
         assertThat(logicalConnection.isCloseable(), is(false)); // we're still enlisted in a suspended transaction
 
         logicalConnection.close(); // doesn't really close, but the caller thinks it did, which is what we want
@@ -378,7 +377,7 @@ final void testBeginSuspendBeginCommitResumeCommit()
         assertThat(physicalConnection2.isClosed(), is(true));
 
         tm.resume(s);
-        
+
         t = tm.getTransaction();
         assertThat(t, sameInstance(s));
         assertThat(t.getStatus(), is(Status.STATUS_ACTIVE));
@@ -392,12 +391,12 @@ final void testBeginSuspendBeginCommitResumeCommit()
 
         tm.commit();
 
-        assertThat(logicalConnection.isClosePending(), is(true));
-
-        assertThat(logicalConnection.isClosed(), is(true)); // returns true only because close is pending
-        assertThat(logicalConnection.delegate().isClosed(), is(false));
+        // Now it should be closed for real.
+        assertThat(logicalConnection.isClosePending(), is(false));
+        assertThat(logicalConnection.isClosed(), is(true));
+        assertThat(logicalConnection.delegate().isClosed(), is(true));
+        assertThat(physicalConnection.isClosed(), is(true));
 
-        assertThat(physicalConnection.isClosed(), is(false)); // logicalConnection was never *really* closed
         assertThat(logicalConnection2.isClosed(), is(true));
         assertThat(physicalConnection2.isClosed(), is(true));