LuciferYang opened a new issue, #64682:
URL: https://github.com/apache/doris/issues/64682

   ### 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<Table> 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!
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to