Skip to content

Commit 2b396b9

Browse files
committed
Fix AB-BA deadlock between SpillableHostStore and SpillableHandle
This fixes a potential deadlock during shutdown where: - Thread 1 (HandleStore.close): Store lock -> Handle lock - Thread 2 (handle.spill): Handle lock -> Store lock The fix ensures consistent lock ordering (Handle lock -> Store lock) by copying handles list under store lock, then processing outside. Affected methods: - HandleStore.close(): Now closes handles outside store lock - SpillableStore.spillableSummary(): Now checks spillable outside store lock Fixes #14201 Signed-off-by: Hongbin Ma (Mahone) <mahongbin@apache.org>
1 parent 2acd1ec commit 2b396b9

1 file changed

Lines changed: 36 additions & 14 deletions

File tree

sql-plugin/src/main/scala/com/nvidia/spark/rapids/spill/SpillFramework.scala

Lines changed: 36 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1337,11 +1337,23 @@ trait HandleStore[T <: StoreHandle] extends AutoCloseable with Logging {
13371337
handles.remove(handle)
13381338
}
13391339

1340-
override def close(): Unit = synchronized {
1341-
handles.forEach(handle => {
1342-
handle.close()
1343-
})
1344-
handles.clear()
1340+
/**
1341+
* Close all handles in the store.
1342+
*
1343+
* Lock ordering: To avoid AB-BA deadlock between store lock and handle lock,
1344+
* we copy the handles list under the store lock, then close handles outside
1345+
* the lock. This ensures we always acquire handle locks without holding the
1346+
* store lock, matching the lock order in handle.spill() which acquires
1347+
* handle lock first, then store lock via removeFromXxxStore().
1348+
*/
1349+
override def close(): Unit = {
1350+
val handlesToClose = synchronized {
1351+
val list = new util.ArrayList[T](handles)
1352+
handles.clear()
1353+
list
1354+
}
1355+
// Close handles outside the store lock to avoid deadlock
1356+
handlesToClose.forEach(_.close())
13451357
}
13461358
}
13471359

@@ -1436,19 +1448,29 @@ trait SpillableStore[T <: SpillableHandle]
14361448
}
14371449
}
14381450

1451+
/**
1452+
* Get a summary of spillable handles in the store.
1453+
*
1454+
* Lock ordering: To avoid AB-BA deadlock between store lock and handle lock,
1455+
* we copy the handles list under the store lock, then check spillable status
1456+
* outside the lock. The handle.spillable check acquires the handle lock,
1457+
* so we must not hold the store lock while calling it.
1458+
*/
14391459
def spillableSummary(): String = {
1460+
val handlesCopy = synchronized {
1461+
new util.ArrayList[T](handles)
1462+
}
14401463
var spillableHandleCount = 0L
14411464
var spillableHandleBytes = 0L
14421465
var totalHandleBytes = 0L
1443-
synchronized {
1444-
handles.forEach(handle => {
1445-
totalHandleBytes += handle.approxSizeInBytes
1446-
if (handle.spillable) {
1447-
spillableHandleCount += 1
1448-
spillableHandleBytes += handle.approxSizeInBytes
1449-
}
1450-
})
1451-
}
1466+
// Iterate outside the store lock to avoid deadlock
1467+
handlesCopy.forEach(handle => {
1468+
totalHandleBytes += handle.approxSizeInBytes
1469+
if (handle.spillable) {
1470+
spillableHandleCount += 1
1471+
spillableHandleBytes += handle.approxSizeInBytes
1472+
}
1473+
})
14521474
s"SpillableStore: ${this.getClass.getSimpleName}, " +
14531475
s"Total Handles: $numHandles, " +
14541476
s"Spillable Handles: $spillableHandleCount, " +

0 commit comments

Comments
 (0)