Skip to content

Commit 1b604c1

Browse files
viiryaHyukjinKwon
authored andcommitted
[SPARK-26265][CORE][FOLLOWUP] Put freePage into a finally block
## What changes were proposed in this pull request? Based on the [comment](apache#23272 (comment)), it seems to be better to put `freePage` into a `finally` block. This patch as a follow-up to do so. ## How was this patch tested? Existing tests. Closes apache#23294 from viirya/SPARK-26265-followup. Authored-by: Liang-Chi Hsieh <viirya@gmail.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
1 parent d25e443 commit 1b604c1

File tree

1 file changed

+30
-27
lines changed

1 file changed

+30
-27
lines changed

core/src/main/java/org/apache/spark/unsafe/map/BytesToBytesMap.java

Lines changed: 30 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -262,36 +262,39 @@ private void advanceToNextPage() {
262262
// reference to the page to free and free it after releasing the lock of `MapIterator`.
263263
MemoryBlock pageToFree = null;
264264

265-
synchronized (this) {
266-
int nextIdx = dataPages.indexOf(currentPage) + 1;
267-
if (destructive && currentPage != null) {
268-
dataPages.remove(currentPage);
269-
pageToFree = currentPage;
270-
nextIdx --;
271-
}
272-
if (dataPages.size() > nextIdx) {
273-
currentPage = dataPages.get(nextIdx);
274-
pageBaseObject = currentPage.getBaseObject();
275-
offsetInPage = currentPage.getBaseOffset();
276-
recordsInPage = UnsafeAlignedOffset.getSize(pageBaseObject, offsetInPage);
277-
offsetInPage += UnsafeAlignedOffset.getUaoSize();
278-
} else {
279-
currentPage = null;
280-
if (reader != null) {
281-
handleFailedDelete();
265+
try {
266+
synchronized (this) {
267+
int nextIdx = dataPages.indexOf(currentPage) + 1;
268+
if (destructive && currentPage != null) {
269+
dataPages.remove(currentPage);
270+
pageToFree = currentPage;
271+
nextIdx--;
282272
}
283-
try {
284-
Closeables.close(reader, /* swallowIOException = */ false);
285-
reader = spillWriters.getFirst().getReader(serializerManager);
286-
recordsInPage = -1;
287-
} catch (IOException e) {
288-
// Scala iterator does not handle exception
289-
Platform.throwException(e);
273+
if (dataPages.size() > nextIdx) {
274+
currentPage = dataPages.get(nextIdx);
275+
pageBaseObject = currentPage.getBaseObject();
276+
offsetInPage = currentPage.getBaseOffset();
277+
recordsInPage = UnsafeAlignedOffset.getSize(pageBaseObject, offsetInPage);
278+
offsetInPage += UnsafeAlignedOffset.getUaoSize();
279+
} else {
280+
currentPage = null;
281+
if (reader != null) {
282+
handleFailedDelete();
283+
}
284+
try {
285+
Closeables.close(reader, /* swallowIOException = */ false);
286+
reader = spillWriters.getFirst().getReader(serializerManager);
287+
recordsInPage = -1;
288+
} catch (IOException e) {
289+
// Scala iterator does not handle exception
290+
Platform.throwException(e);
291+
}
290292
}
291293
}
292-
}
293-
if (pageToFree != null) {
294-
freePage(pageToFree);
294+
} finally {
295+
if (pageToFree != null) {
296+
freePage(pageToFree);
297+
}
295298
}
296299
}
297300

0 commit comments

Comments
 (0)