Skip to content

Commit

Permalink
Fixes erroneous closing behavior in JtaConnection.java (helidon-io#6321)
Browse files Browse the repository at this point in the history 
* Fixes erroneous closing behavior in JtaConnection.java

Signed-off-by: Laird Nelson <laird.nelson@oracle.com>
  • Loading branch information
@ljnelson
ljnelson authored Mar 2, 2023
1 parent 0706517 commit b5a744f
Show file tree
Hide file tree
Showing 3 changed files with 148 additions and 56 deletions.
154 changes: 130 additions & 24 deletions ...dbc/jdbc/src/main/java/io/helidon/integrations/jdbc/ConditionallyCloseableConnection.java
View file
Original file line number Diff line number Diff line change
@@ -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.
Expand Down Expand Up @@ -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;


/*
Expand Down Expand Up @@ -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;
}


Expand Down Expand Up @@ -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();
}
}

Expand Down Expand Up @@ -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();
}
}

/**
Expand Down Expand Up @@ -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();
}
}

Expand All @@ -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
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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;

}

}
37 changes: 12 additions & 25 deletions integrations/jta/jdbc/src/main/java/io/helidon/integrations/jta/jdbc/JtaConnection.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.)
Expand Down Expand Up @@ -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");
Expand Down
13 changes: 6 additions & 7 deletions integrations/jta/jdbc/src/test/java/io/helidon/integrations/jta/jdbc/TestJtaConnection.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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));
Expand All @@ -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));

Expand Down

0 comments on commit b5a744f

Please sign in to comment.