Skip to content

[SPARK-11424] Guard against double-close() of RecordReaders #9382

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

[SPARK-11424] Guard against double-close() of RecordReaders #9382

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
087aa63
Guard against double-close() of RecordReaders.
JoshRosen Oct 30, 2015
ae46cf4
Merge remote-tracking branch 'origin/master' into hadoop-decompressor…
JoshRosen Oct 30, 2015
5ec97d7
Add SqlNewHadoopRDD.unsetInputFileName() that I accidentally deleted.
JoshRosen Oct 31, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 15 additions & 8 deletions core/src/main/scala/org/apache/spark/rdd/HadoopRDD.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,8 +251,21 @@ class HadoopRDD[K, V](
}

override def close() {
try {
reader.close()
if (reader != null) {
// Close the reader and release it. Note: it's very important that we don't close the
// reader more than once, since that exposes us to MAPREDUCE-5918 when running against
// Hadoop 1.x and older Hadoop 2.x releases. That bug can lead to non-deterministic
// corruption issues when reading compressed input.
try {
reader.close()
} catch {
case e: Exception =>
if (!ShutdownHookManager.inShutdown()) {
logWarning("Exception in RecordReader.close()", e)
}
} finally {
reader = null
}
if (bytesReadCallback.isDefined) {
inputMetrics.updateBytesRead()
} else if (split.inputSplit.value.isInstanceOf[FileSplit] ||
Expand All @@ -266,12 +279,6 @@ class HadoopRDD[K, V](
logWarning("Unable to get input size to set InputMetrics for task", e)
}
}
} catch {
case e: Exception => {
if (!ShutdownHookManager.inShutdown()) {
logWarning("Exception in RecordReader.close()", e)
}
}
}
}
}
Expand Down
44 changes: 23 additions & 21 deletions core/src/main/scala/org/apache/spark/rdd/NewHadoopRDD.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,30 +184,32 @@ class NewHadoopRDD[K, V](
}

private def close() {
try {
if (reader != null) {
// Close reader and release it
if (reader != null) {
// Close the reader and release it. Note: it's very important that we don't close the
// reader more than once, since that exposes us to MAPREDUCE-5918 when running against
// Hadoop 1.x and older Hadoop 2.x releases. That bug can lead to non-deterministic
// corruption issues when reading compressed input.
try {
reader.close()
reader = null

if (bytesReadCallback.isDefined) {
inputMetrics.updateBytesRead()
} else if (split.serializableHadoopSplit.value.isInstanceOf[FileSplit] ||
split.serializableHadoopSplit.value.isInstanceOf[CombineFileSplit]) {
// If we can't get the bytes read from the FS stats, fall back to the split size,
// which may be inaccurate.
try {
inputMetrics.incBytesRead(split.serializableHadoopSplit.value.getLength)
} catch {
case e: java.io.IOException =>
logWarning("Unable to get input size to set InputMetrics for task", e)
} catch {
case e: Exception =>
if (!ShutdownHookManager.inShutdown()) {
logWarning("Exception in RecordReader.close()", e)
}
}
} finally {
reader = null
}
} catch {
case e: Exception => {
if (!ShutdownHookManager.inShutdown()) {
logWarning("Exception in RecordReader.close()", e)
if (bytesReadCallback.isDefined) {
inputMetrics.updateBytesRead()
} else if (split.serializableHadoopSplit.value.isInstanceOf[FileSplit] ||
split.serializableHadoopSplit.value.isInstanceOf[CombineFileSplit]) {
// If we can't get the bytes read from the FS stats, fall back to the split size,
// which may be inaccurate.
try {
inputMetrics.incBytesRead(split.serializableHadoopSplit.value.getLength)
} catch {
case e: java.io.IOException =>
logWarning("Unable to get input size to set InputMetrics for task", e)
}
}
}
Expand Down
47 changes: 25 additions & 22 deletions core/src/main/scala/org/apache/spark/rdd/SqlNewHadoopRDD.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,32 +189,35 @@ private[spark] class SqlNewHadoopRDD[V: ClassTag](
}

private def close() {
try {
if (reader != null) {
if (reader != null) {
SqlNewHadoopRDD.unsetInputFileName()
// Close the reader and release it. Note: it's very important that we don't close the
// reader more than once, since that exposes us to MAPREDUCE-5918 when running against
// Hadoop 1.x and older Hadoop 2.x releases. That bug can lead to non-deterministic
// corruption issues when reading compressed input.
Copy link
Contributor Author

@JoshRosen JoshRosen Oct 31, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, this is why the test was failing.

try {
reader.close()
reader = null

SqlNewHadoopRDD.unsetInputFileName()

if (bytesReadCallback.isDefined) {
inputMetrics.updateBytesRead()
} else if (split.serializableHadoopSplit.value.isInstanceOf[FileSplit] ||
split.serializableHadoopSplit.value.isInstanceOf[CombineFileSplit]) {
// If we can't get the bytes read from the FS stats, fall back to the split size,
// which may be inaccurate.
try {
inputMetrics.incBytesRead(split.serializableHadoopSplit.value.getLength)
} catch {
case e: java.io.IOException =>
logWarning("Unable to get input size to set InputMetrics for task", e)
} catch {
case e: Exception =>
if (!ShutdownHookManager.inShutdown()) {
logWarning("Exception in RecordReader.close()", e)
}
}
} finally {
reader = null
}
} catch {
case e: Exception =>
if (!ShutdownHookManager.inShutdown()) {
logWarning("Exception in RecordReader.close()", e)
if (bytesReadCallback.isDefined) {
inputMetrics.updateBytesRead()
} else if (split.serializableHadoopSplit.value.isInstanceOf[FileSplit] ||
split.serializableHadoopSplit.value.isInstanceOf[CombineFileSplit]) {
// If we can't get the bytes read from the FS stats, fall back to the split size,
// which may be inaccurate.
try {
inputMetrics.incBytesRead(split.serializableHadoopSplit.value.getLength)
} catch {
case e: java.io.IOException =>
logWarning("Unable to get input size to set InputMetrics for task", e)
}
}
}
}
}
Expand Down
4 changes: 3 additions & 1 deletion core/src/main/scala/org/apache/spark/util/NextIterator.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,10 @@ private[spark] abstract class NextIterator[U] extends Iterator[U] {
*/
def closeIfNeeded() {
if (!closed) {
Copy link
Contributor Author

@JoshRosen JoshRosen Oct 31, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, this use of nextIterator() explains why this bug was able to occur on 1.4.x: the first close() call, which throws an exception, comes from NextIterator, and the second comes from the task completion listener.

close()
// Note: it's important that we set closed = true before calling close(), since setting it
// afterwards would permit us to call close() multiple times if close() threw an exception.
closed = true
close()
}
}

Expand Down