apache / apache/doris

[Bug] MetaLockUtils.commitLockTables leaks acquired commit locks on rollback (unlocks wrong index)

Open
#64,682 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
Java
Stars
15.9k
Forks
3.9k
Avg merge
2d 23h
Merged PRs (30d)
520

Description

### Search before asking

- [x] I had searched in the [issues](https://github.com/apache/doris/issues) and found no similar issues.

### Version

master (`MetaLockUtils.commitLockTables`).

### What's Wrong?

`MetaLockUtils.commitLockTables` has a rollback bug: when `commitLock()` throws for the table at index `i`, the rollback loop iterates `j` from `i-1` down to `0` but unlocks `tableList.get(i)` (the table that failed to lock) instead of `tableList.get(j)` (the previously locked tables):

```java
public static void commitLockTables(List tableList) {
for (int i = 0; i < tableList.size(); i++) {
try {
tableList.get(i).commitLock();
} catch (Exception e) {
for (int j = i - 1; j >= 0; j--) {
tableList.get(i).commitUnlock(); // should be get(j)
}
}
}
}
```

Consequences on the failure path:
- Commit locks already acquired for tables `0..i-1` are never released (leak).
- The failing table (`i`) is `commitUnlock()`-ed repeatedly on a lock it does not hold.
- The exception is swallowed and the outer loop keeps trying to lock the remaining tables.

The correct siblings `tryCommitLockTables` and `writeLockTablesOrMetaException` use `get(j)` and propagate the failure.

Note: in normal operation `commitLock()` → `MonitoredReentrantLock.lock()` does not throw, so this `catch` is defensive and the bug is latent; the fix makes the rare failure path correct.

### What You Expected?

On a commit-lock acquisition failure, the locks already acquired should be released (`get(j)`) and the failure propagated, instead of leaking locks and silently continuing.

### How to Reproduce?

Code inspection: `commitLockTables` unlocks `get(i)` instead of `get(j)` in its rollback loop, diverging from every other lock helper in the class. A unit test that injects a table whose `commitLock()` throws shows the previously-locked table is left locked.

### Anything Else?

Fix proposed in PR #64676.

### Are you willing to submit PR?

- [x] Yes I am willing to submit a PR!

Contributor guide

Open the contributing guide

Research direction

Start in MetaLockUtils.commitLockTables and compare its rollback loop with tryCommitLockTables and writeLockTablesOrMetaException. Use a unit test with a table whose commitLock() throws; done means previously acquired tables are unlocked and the failure is propagated rather than swallowed.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
databases
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.