diff --git a/core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorter.java b/core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorter.java index af5a934b7da6..a49ccf60333f 100644 --- a/core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorter.java +++ b/core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorter.java @@ -573,19 +573,29 @@ public boolean hasNext() { @Override public void loadNext() throws IOException { - synchronized (this) { - loaded = true; - if (nextUpstream != null) { - // Just consumed the last record from in memory iterator - if (lastPage != null) { - freePage(lastPage); - lastPage = null; + MemoryBlock pageToFree = null; + try { + synchronized (this) { + loaded = true; + if (nextUpstream != null) { + // Just consumed the last record from in memory iterator + if (lastPage != null) { + // Do not free the page here, while we are locking `SpillableIterator`. The `freePage` + // method locks the `TaskMemoryManager`, and it's a bad idea to lock 2 objects in + // sequence. We may hit dead lock if another thread locks `TaskMemoryManager` and + // `SpillableIterator` in sequence, which may happen in + // `TaskMemoryManager.acquireExecutionMemory`. + pageToFree = lastPage; + lastPage = null; + } + upstream = nextUpstream; + nextUpstream = null; } - upstream = nextUpstream; - nextUpstream = null; + numRecords--; + upstream.loadNext(); } - numRecords--; - upstream.loadNext(); + } finally { + if (pageToFree != null) freePage(pageToFree); } }