Skip to content

IGNITE-28675 Fix flaky TxDeadlockCauseTest #testCause, #testCauseSeveralNodes #13136

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
zstan wants to merge 7 commits into
base: master
Choose a base branch
from
Open

IGNITE-28675 Fix flaky TxDeadlockCauseTest #testCause, #testCauseSeveralNodes #13136

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
94b11ec
IGNITE-28675 Fix flaky TxDeadlockCauseTest#testCause
zstan May 13, 2026
f764e52
fix final change through reflection
zstan May 15, 2026
c4d175a
fix testCauseSeveralNodes
zstan May 15, 2026
d57f6c3
style fix
zstan May 20, 2026
2fd5bb3
test enhacement
zstan May 22, 2026
41500b0
fix after review
zstan May 27, 2026
0a8be5a
fix after review 2
zstan May 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 18 additions & 38 deletions ...in/java/org/apache/ignite/internal/processors/cache/transactions/TxDeadlockDetection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import org.apache.ignite.internal.util.future.GridFutureAdapter;
import org.apache.ignite.internal.util.tostring.GridToStringExclude;
import org.apache.ignite.internal.util.tostring.GridToStringInclude;
import org.apache.ignite.internal.util.typedef.F;
import org.apache.ignite.internal.util.typedef.T2;
import org.apache.ignite.internal.util.typedef.internal.S;
import org.apache.ignite.internal.util.typedef.internal.U;
Expand All @@ -55,7 +56,7 @@ public class TxDeadlockDetection {
public static final int DFLT_TX_DEADLOCK_DETECTION_TIMEOUT = 60000;

/** Deadlock detection maximum iterations. */
private static int deadLockTimeout =
private static final int DEAD_LOCK_TIMEOUT =
Copy link
Copy Markdown
Contributor

@chesnokoff chesnokoff May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's roll back adding the final qualifier here, because it changes the semantics of TxDeadlockDetectionNoHangsTest and is not the main goal of this patch.

Copy link
Copy Markdown
Contributor

@chesnokoff chesnokoff May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and roll back renaming too

Copy link
Copy Markdown
Contributor Author

@zstan zstan May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

disagree, why do we need to support non-final definition only for test cases ?
test work properly well without this reflection changes, i run it multiple times, you can check it through TC

Copy link
Copy Markdown
Contributor

@chesnokoff chesnokoff May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My position is not really about final/non-final definition itself. My point is that it is usually safer to keep PRs as minimal as possible for a specific purpose. The main goal here is to fix TxDeadlockCauseTest, and for now it is not very clear why we also need to refactor an unrelated field and affect existing test behavior.

Green TC runs are definitely a good sign, but some flaky tests fail once in weeks or even months. That's why I'd prefer to avoid introducing additional assumptions here unless this change is really required for the fix itself.

Could you provide more reasoning on why it is safe to remove before/afterTests from TxDeadlockDetectionNoHangsTest?

Copy link
Copy Markdown
Contributor Author

@zstan zstan May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appent additional checks into test, plz check last commit
Also, tests restore after for hardcoded - 60000, that equal to current default but can be changed in future thus there can be a situation when test restores not a default.

getInteger(IGNITE_TX_DEADLOCK_DETECTION_TIMEOUT, DFLT_TX_DEADLOCK_DETECTION_TIMEOUT);

/** Sequence. */
Expand All @@ -80,7 +81,7 @@ public TxDeadlockDetection(GridCacheSharedContext<?, ?> cctx) {
*
* @param tx Target tx.
* @param keys Keys.
* @return {@link TxDeadlock} if found, otherwise - {@code null}.
* @return {@link TxDeadlockFuture} future.
*/
TxDeadlockFuture detectDeadlock(IgniteInternalTx tx, Set<IgniteTxKey> keys) {
GridCacheVersion txId = tx.nearXidVersion();
Expand All @@ -101,7 +102,7 @@ TxDeadlockFuture detectDeadlock(IgniteInternalTx tx, Set<IgniteTxKey> keys) {
* @param wfg Wait-for-graph.
* @param txId Tx ID - start vertex for cycle search in graph.
*/
static List<GridCacheVersion> findCycle(Map<GridCacheVersion, Set<GridCacheVersion>> wfg, GridCacheVersion txId) {
static @Nullable List<GridCacheVersion> findCycle(Map<GridCacheVersion, Set<GridCacheVersion>> wfg, GridCacheVersion txId) {
if (wfg == null || wfg.isEmpty())
return null;

Expand Down Expand Up @@ -181,7 +182,7 @@ static class TxDeadlockFuture extends GridFutureAdapter<TxDeadlock> {

/** Pending keys. */
@GridToStringInclude
private Map<UUID, Set<IgniteTxKey>> pendingKeys = new HashMap<>();
private final Map<UUID, Set<IgniteTxKey>> pendingKeys = new HashMap<>();

/** Nodes queue. */
@GridToStringInclude
Expand Down Expand Up @@ -233,7 +234,7 @@ private TxDeadlockFuture(GridCacheSharedContext cctx,
this.topVer = topVer;
this.keys = keys;

if (deadLockTimeout > 0) {
if (DEAD_LOCK_TIMEOUT > 0) {
timeoutObj = new DeadlockTimeoutObject();

cctx.time().addTimeoutObject(timeoutObj);
Expand Down Expand Up @@ -322,7 +323,7 @@ private void detect(TxLocksResponse res) {
* Maps tx keys on nodes. Key can be mapped on some node if this node is primary for given key or
* node is near for transaction that holds or requests lock for key.
*
* Key will not be be mapped to node if both key and node are already handled.
* Key will not be mapped to node if both key and node are already handled.
*
* @param txKeys Tx keys.
* @param txLocks Tx locks.
Expand All @@ -348,10 +349,7 @@ private void mapTxKeys(@Nullable Set<IgniteTxKey> txKeys, Map<IgniteTxKey, List<
// Process this node earlier than other in order to optimize amount of requests.
preferredNodes.add(nodeId);

Set<IgniteTxKey> mappedKeys = pendingKeys.get(nodeId);

if (mappedKeys == null)
pendingKeys.put(nodeId, mappedKeys = new HashSet<>());
Set<IgniteTxKey> mappedKeys = pendingKeys.computeIfAbsent(nodeId, k -> new HashSet<>());

mappedKeys.add(txKey);
}
Expand All @@ -363,10 +361,7 @@ private void mapTxKeys(@Nullable Set<IgniteTxKey> txKeys, Map<IgniteTxKey, List<
else
nodesQueue.addLast(nearNodeId);

Set<IgniteTxKey> mappedKeys = pendingKeys.get(nearNodeId);

if (mappedKeys == null)
pendingKeys.put(nearNodeId, mappedKeys = new HashSet<>());
Set<IgniteTxKey> mappedKeys = pendingKeys.computeIfAbsent(nearNodeId, k -> new HashSet<>());

mappedKeys.add(txKey);
}
Expand All @@ -387,10 +382,7 @@ private void mapTxKeys(@Nullable Set<IgniteTxKey> txKeys, Map<IgniteTxKey, List<

nodesQueue.addLast(nodeId);

Set<IgniteTxKey> mappedKeys = pendingKeys.get(nodeId);

if (mappedKeys == null)
pendingKeys.put(nodeId, mappedKeys = new HashSet<>());
Set<IgniteTxKey> mappedKeys = pendingKeys.computeIfAbsent(nodeId, k -> new HashSet<>());

mappedKeys.add(txKey);
}
Expand All @@ -417,15 +409,15 @@ private UUID primary(IgniteTxKey txKey) {
private void merge(TxLocksResponse res) {
Map<IgniteTxKey, List<TxLock>> txLocks = res.txLocks();

if (txLocks == null || txLocks.isEmpty())
if (F.isEmpty(txLocks))
return;

for (Map.Entry<IgniteTxKey, List<TxLock>> e : txLocks.entrySet()) {
IgniteTxKey txKey = e.getKey();

List<TxLock> lockList = e.getValue();

if (lockList != null && !lockList.isEmpty()) {
if (!F.isEmpty(lockList)) {
for (TxLock lock : lockList) {
if (lock.owner() || lock.candiate()) {
if (txs.get(lock.txId()) == null)
Expand All @@ -435,18 +427,12 @@ private void merge(TxLocksResponse res) {
if (lock.owner()) {
GridCacheVersion txId = lock.txId();

Set<IgniteTxKey> keys = txLockedKeys.get(txId);

if (keys == null)
txLockedKeys.put(txId, keys = new HashSet<>());
Set<IgniteTxKey> keys = txLockedKeys.computeIfAbsent(txId, k -> new HashSet<>());

keys.add(txKey);
}
else if (lock.candiate()) {
Set<GridCacheVersion> txs = txRequestedKeys.get(txKey);

if (txs == null)
txRequestedKeys.put(txKey, txs = new HashSet<>());
Set<GridCacheVersion> txs = txRequestedKeys.computeIfAbsent(txKey, k -> new HashSet<>());

txs.add(lock.txId());
}
Expand All @@ -473,10 +459,7 @@ private void updateWaitForGraph(Map<IgniteTxKey, List<TxLock>> txLocks) {
txOwner = lock.txId();

if (keys.contains(e.getKey()) && !txId.equals(lock.txId())) {
Set<GridCacheVersion> waitingTxs = wfg.get(txId);

if (waitingTxs == null)
wfg.put(txId, waitingTxs = new HashSet<>());
Set<GridCacheVersion> waitingTxs = wfg.computeIfAbsent(txId, k -> new HashSet<>());

waitingTxs.add(lock.txId());
}
Expand All @@ -487,10 +470,7 @@ private void updateWaitForGraph(Map<IgniteTxKey, List<TxLock>> txLocks) {
if (lock.candiate() || lock.owner()) {
GridCacheVersion txId0 = lock.txId();

Set<GridCacheVersion> waitForTxs = wfg.get(txId0);

if (waitForTxs == null)
wfg.put(txId0, waitForTxs = new HashSet<>());
Set<GridCacheVersion> waitForTxs = wfg.computeIfAbsent(txId0, k -> new HashSet<>());

waitForTxs.add(txOwner);
}
Expand Down Expand Up @@ -562,7 +542,7 @@ private class DeadlockTimeoutObject extends GridTimeoutObjectAdapter {
* Default constructor.
*/
DeadlockTimeoutObject() {
super(deadLockTimeout);
super(DEAD_LOCK_TIMEOUT);
}

/** {@inheritDoc} */
Expand All @@ -571,7 +551,7 @@ private class DeadlockTimeoutObject extends GridTimeoutObjectAdapter {

IgniteLogger log = cctx.kernalContext().log(this.getClass());

U.warn(log, "Deadlock detection was timed out [timeout=" + deadLockTimeout + ", fut=" + this + ']');
U.warn(log, "Deadlock detection was timed out [timeout=" + DEAD_LOCK_TIMEOUT + ", fut=" + this + ']');

onDone();
}
Expand Down
12 changes: 8 additions & 4 deletions ...st/java/org/apache/ignite/internal/processors/cache/transactions/TxDeadlockCauseTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@
import java.util.concurrent.atomic.AtomicReference;
import org.apache.ignite.Ignite;
import org.apache.ignite.IgniteCache;
import org.apache.ignite.IgniteCheckedException;
import org.apache.ignite.cache.CacheAtomicityMode;
import org.apache.ignite.configuration.CacheConfiguration;
import org.apache.ignite.configuration.DataRegionConfiguration;
Expand Down Expand Up @@ -165,6 +164,9 @@ private void checkCauseObject(
final TransactionIsolation isolation,
final boolean oneOp
) throws Exception {
if (nodes > 1)
awaitPartitionMapExchange();

final Ignite ignite = grid(new Random().nextInt(nodes));

final IgniteCache<Integer, Account> cache = ignite.cache(DEFAULT_CACHE_NAME);
Expand All @@ -183,7 +185,7 @@ private void checkCauseObject(
final CyclicBarrier barrier = new CyclicBarrier(2);

IgniteInternalFuture<Long> fut = GridTestUtils.runMultiThreadedAsync(new CAX() {
@Override public void applyx() throws IgniteCheckedException {
@Override public void applyx() {
try (Transaction tx = ignite.transactions().txStart(TransactionConcurrency.PESSIMISTIC, isolation,
timeout, keys.size())) {

Expand All @@ -204,7 +206,9 @@ private void checkCauseObject(
tx.commit();
}
catch (Exception e) {
ex.compareAndSet(null, e);
// TransactionDeadlockException raised at least for one transaction involved in the deadlock
Copy link
Copy Markdown
Contributor Author

@zstan zstan May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is from our own documentation

if (X.hasCause(e, TransactionDeadlockException.class))
ex.compareAndSet(null, e);
}
}
}, 2, "tx");
Expand Down Expand Up @@ -268,7 +272,7 @@ static class Account implements Serializable {
/**
* Change balance by specified amount.
*
* @param amount Amount to add to balance (may be negative).
* @param amount Amount to add to balance (maybe negative).
*/
void update(double amount) {
balance += amount;
Expand Down
36 changes: 21 additions & 15 deletions .../apache/ignite/internal/processors/cache/transactions/TxDeadlockDetectionNoHangsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import java.util.concurrent.ThreadLocalRandom;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.function.Consumer;
import org.apache.ignite.Ignite;
import org.apache.ignite.IgniteCache;
import org.apache.ignite.IgniteCheckedException;
Expand All @@ -32,14 +33,13 @@
import org.apache.ignite.internal.IgniteKernal;
import org.apache.ignite.internal.util.typedef.internal.U;
import org.apache.ignite.testframework.GridTestUtils;
import org.apache.ignite.testframework.ListeningTestLogger;
import org.apache.ignite.testframework.junits.WithSystemProperty;
import org.apache.ignite.testframework.junits.common.GridCommonAbstractTest;
import org.apache.ignite.transactions.Transaction;
import org.apache.ignite.transactions.TransactionConcurrency;
import org.junit.Test;

import static org.apache.ignite.IgniteSystemProperties.IGNITE_TX_DEADLOCK_DETECTION_TIMEOUT;
import static org.apache.ignite.IgniteSystemProperties.getInteger;
import static org.apache.ignite.transactions.TransactionConcurrency.OPTIMISTIC;
import static org.apache.ignite.transactions.TransactionConcurrency.PESSIMISTIC;
import static org.apache.ignite.transactions.TransactionIsolation.REPEATABLE_READ;
Expand All @@ -54,6 +54,18 @@ public class TxDeadlockDetectionNoHangsTest extends GridCommonAbstractTest {
/** Cache. */
private static final String CACHE = "cache";

/** Log listener. */
private final ListeningTestLogger listeningLog = new ListeningTestLogger(log);

/** Deadlock timeout, it`s unexpected during these tests. */
private static final AtomicBoolean DEAD_LOCK_FLAG = new AtomicBoolean();

/** */
private static final Consumer<String> DEAD_LOCK_LSNR = s -> {
if (s.contains("Deadlock detection was timed out"))
DEAD_LOCK_FLAG.set(true);
};

/** {@inheritDoc} */
@SuppressWarnings("unchecked")
@Override protected IgniteConfiguration getConfiguration(String igniteInstanceName) throws Exception {
Expand All @@ -68,6 +80,11 @@ public class TxDeadlockDetectionNoHangsTest extends GridCommonAbstractTest {

cfg.setCacheConfiguration(ccfg);

assertFalse(DEAD_LOCK_FLAG.get());

listeningLog.registerListener(DEAD_LOCK_LSNR);
cfg.setGridLogger(listeningLog);

return cfg;
}

Expand All @@ -82,20 +99,9 @@ public class TxDeadlockDetectionNoHangsTest extends GridCommonAbstractTest {
@Override protected void afterTest() throws Exception {
super.afterTest();

stopAllGrids();
}
assertFalse(DEAD_LOCK_FLAG.get());

/** {@inheritDoc} */
@Override protected void beforeTestsStarted() throws Exception {
super.beforeTestsStarted();

GridTestUtils.setFieldValue(TxDeadlockDetection.class, "deadLockTimeout", (int)(getTestTimeout() * 2));
}

/** {@inheritDoc} */
@Override protected void afterTestsStopped() throws Exception {
GridTestUtils.setFieldValue(TxDeadlockDetection.class, "deadLockTimeout",
getInteger(IGNITE_TX_DEADLOCK_DETECTION_TIMEOUT, 60000));
stopAllGrids();
}

/** {@inheritDoc} */
Expand Down
Loading