We have a thinko around recording frontend transaction, which leads
to us always cancelling the entire chain. This is not correct, as
evidenced by existing tests.
Correct the book keeping and refactor cancelTransaction() to allow
returning correct result of operation and handle the case where backend
refuses to cancel.
JIRA: MDSAL-756
Change-Id: I553e4de2d09acedf67af63bc292fae0bb5dfed78
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>
(cherry picked from commit
e781d0af7effb145dc69163744d1a7f02c200d8e)
* interface so we have a simple way of propagating the result.
*/
final class PingPongTransaction implements FutureCallback<CommitInfo> {
* interface so we have a simple way of propagating the result.
*/
final class PingPongTransaction implements FutureCallback<CommitInfo> {
+ private final @NonNull SettableFuture<CommitInfo> future = SettableFuture.create();
+ private final @NonNull FluentFuture<CommitInfo> fluent = FluentFuture.from(future);
private final @NonNull DOMDataTreeReadWriteTransaction delegate;
private final @NonNull DOMDataTreeReadWriteTransaction delegate;
- private final @NonNull SettableFuture<CommitInfo> future;
- private final @NonNull FluentFuture<CommitInfo> fluent;
private @Nullable DOMDataTreeReadWriteTransaction frontendTransaction;
PingPongTransaction(final DOMDataTreeReadWriteTransaction delegate) {
this.delegate = requireNonNull(delegate);
private @Nullable DOMDataTreeReadWriteTransaction frontendTransaction;
PingPongTransaction(final DOMDataTreeReadWriteTransaction delegate) {
this.delegate = requireNonNull(delegate);
- future = SettableFuture.create();
- fluent = FluentFuture.from(future);
}
@NonNull DOMDataTreeReadWriteTransaction getTransaction() {
}
@NonNull DOMDataTreeReadWriteTransaction getTransaction() {
}
void recordFrontendTransaction(final DOMDataTreeReadWriteTransaction tx) {
}
void recordFrontendTransaction(final DOMDataTreeReadWriteTransaction tx) {
- if (frontendTransaction != null) {
- frontendTransaction = tx;
+ if (frontendTransaction == null) {
+ frontendTransaction = requireNonNull(tx);
*
* @param tx Backend shared transaction
* @param frontendTx transaction
*
* @param tx Backend shared transaction
* @param frontendTx transaction
- * @param isOpen indicator whether the transaction was already closed
+ * @return {@code true} if the transaction was cancelled successfully
- synchronized void cancelTransaction(final PingPongTransaction tx,
+ synchronized boolean cancelTransaction(final PingPongTransaction tx,
final DOMDataTreeReadWriteTransaction frontendTx) {
// Attempt to unlock the operation.
final Object witness = LOCKED_TX.compareAndExchange(this, tx, null);
final DOMDataTreeReadWriteTransaction frontendTx) {
// Attempt to unlock the operation.
final Object witness = LOCKED_TX.compareAndExchange(this, tx, null);
if (failed) {
// The transaction has failed, this is probably the user just clearing up the transaction they had. We have
// already cancelled the transaction anyway,
if (failed) {
// The transaction has failed, this is probably the user just clearing up the transaction they had. We have
// already cancelled the transaction anyway,
- return;
- } else if (!backendCancelled) {
- LOG.warn("Backend transaction cannot be cancelled during cancellation of {}, attempting to continue", tx);
- // We have dealt with canceling the backend transaction and have unlocked the transaction. Since we are still
+ // We have dealt with cancelling the backend transaction and have unlocked the transaction. Since we are still
// inside the synchronized block, any allocations are blocking on the slow path. Now we have to decide the fate
// of this transaction chain.
//
// If there are no other frontend transactions in this batch we are aligned with backend state and we can
// continue processing.
if (frontendTx.equals(tx.getFrontendTransaction())) {
// inside the synchronized block, any allocations are blocking on the slow path. Now we have to decide the fate
// of this transaction chain.
//
// If there are no other frontend transactions in this batch we are aligned with backend state and we can
// continue processing.
if (frontendTx.equals(tx.getFrontendTransaction())) {
- LOG.debug("Cancelled transaction {} was head of the batch, resuming processing", tx);
- return;
+ if (backendCancelled) {
+ LOG.debug("Cancelled transaction {} was head of the batch, resuming processing", tx);
+ return true;
+ }
+
+ // Backend refused to cancel the transaction. Reinstate it to locked state.
+ final Object reinstateWitness = LOCKED_TX.compareAndExchange(this, null, tx);
+ verify(reinstateWitness == null, "Reinstating transaction %s collided with locked transaction %s", tx,
+ reinstateWitness);
+ return false;
+ }
+
+ if (!backendCancelled) {
+ LOG.warn("Backend transaction cannot be cancelled during cancellation of {}, attempting to continue", tx);
}
// There are multiple frontend transactions in this batch. We have to report them as failed, which dooms this
}
// There are multiple frontend transactions in this batch. We have to report them as failed, which dooms this
// and mark the fact that we should be turning its completion into a failure.
deadTx = Map.entry(tx, new CancellationException("Transaction " + frontendTx + " canceled").fillInStackTrace());
delegate.close();
// and mark the fact that we should be turning its completion into a failure.
deadTx = Map.entry(tx, new CancellationException("Transaction " + frontendTx + " canceled").fillInStackTrace());
delegate.close();
@Override
public boolean cancel() {
@Override
public boolean cancel() {
- if (isOpen) {
- cancelTransaction(tx, this);
+ if (isOpen && cancelTransaction(tx, this)) {
isOpen = false;
return true;
}
isOpen = false;
return true;
}
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertSame;
import static org.junit.Assert.assertThrows;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertSame;
import static org.junit.Assert.assertThrows;
-import static org.junit.Assert.assertTrue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.doNothing;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.doNothing;
private void assertSimpleCancel(final boolean result) {
final var tx = pingPong.newWriteOnlyTransaction();
private void assertSimpleCancel(final boolean result) {
final var tx = pingPong.newWriteOnlyTransaction();
- doNothing().when(chain).close();
doReturn(result).when(rwTx).cancel();
doReturn(result).when(rwTx).cancel();
- doReturn("mock").when(rwTx).toString();
-
- // FIXME: it seems we are doing the wrong, we should see 'result' returned here
- assertTrue(tx.cancel());
-
+ assertEquals(result, tx.cancel());
}
private static <T> T assertDone(final FluentFuture<T> future) {
}
private static <T> T assertDone(final FluentFuture<T> future) {