This is an automated email from the ASF dual-hosted git repository.
LuciferYang pushed a commit to branch branch-4.x
in repository https://gitbox.apache.org/repos/asf/spark.git
The following commit(s) were added to refs/heads/branch-4.x by this push:
new 5cacd4d7ee29 [SPARK-57504][CORE] Avoid a potential NPE in
`IndexShuffleBlockResolver.getChecksums` when the checksum file cannot be opened
5cacd4d7ee29 is described below
commit 5cacd4d7ee294afe61795738f3472975248d11aa
Author: YangJie <[email protected]>
AuthorDate: Fri Jun 19 10:32:51 2026 +0800
[SPARK-57504][CORE] Avoid a potential NPE in
`IndexShuffleBlockResolver.getChecksums` when the checksum file cannot be opened
### What changes were proposed in this pull request?
`IndexShuffleBlockResolver.getChecksums` opens the checksum file with `var
in: DataInputStream = null` inside a `try` and closes it in `finally {
in.close() }`. The method already catches `IOException`/`EOFException` to
return `null`, so it is meant to degrade gracefully when the file cannot be
read. But the `finally` is not null-safe: if the failure originates in the
stream constructor (before `in` is assigned), it dereferences `null` and throws
a `NullPointerException` that masks th [...]
The two sibling methods in the same file already handle constructor failure
correctly: `checkIndexAndDataFile` constructs the stream in its own
`try/catch`, and `getMergedBlockData` uses `Utils.tryWithResource`. This change
makes `getChecksums` consistent by using `Utils.tryWithResource`, which
evaluates the constructor before its own `try`, so a constructor `IOException`
reaches the existing `catch` and the method returns `null` as intended.
### Why are the changes needed?
`getChecksums` has a latent `NullPointerException`: the non-null-safe
`finally { in.close() }` would mask the real error if opening the checksum file
fails at construction time, which is inconsistent with the rest of the file. No
common code path is known to trigger it today; this is a robustness and
consistency improvement that removes the latent NPE and aligns the method with
its siblings.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Added a unit test in `IndexShuffleBlockResolverSuite` that forces the
stream constructor to fail (a checksum file whose `exists()` returns true but
which cannot be opened) and asserts `getChecksums` returns `null` instead of
throwing. The test fails on the old code (NPE) and passes with the fix.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 4.8)
Closes #56564 from LuciferYang/core-getChecksums-npe-fix.
Authored-by: YangJie <[email protected]>
Signed-off-by: yangjie01 <[email protected]>
(cherry picked from commit e597414d0a3b1a16931c49bc4fa5c4e936f859db)
Signed-off-by: yangjie01 <[email protected]>
---
.../spark/shuffle/IndexShuffleBlockResolver.scala | 19 +++++++++----------
.../shuffle/sort/IndexShuffleBlockResolverSuite.scala | 16 ++++++++++++++++
2 files changed, 25 insertions(+), 10 deletions(-)
diff --git
a/core/src/main/scala/org/apache/spark/shuffle/IndexShuffleBlockResolver.scala
b/core/src/main/scala/org/apache/spark/shuffle/IndexShuffleBlockResolver.scala
index a46c23447f84..39a7aa4e7ab1 100644
---
a/core/src/main/scala/org/apache/spark/shuffle/IndexShuffleBlockResolver.scala
+++
b/core/src/main/scala/org/apache/spark/shuffle/IndexShuffleBlockResolver.scala
@@ -576,22 +576,21 @@ private[spark] class IndexShuffleBlockResolver(
private[shuffle] def getChecksums(checksumFile: File, blockNum: Int):
Array[Long] = {
if (!checksumFile.exists()) return null
- val checksums = new ArrayBuffer[Long]
// Read the checksums of blocks
- var in: DataInputStream = null
try {
- in = new DataInputStream(new NioBufferedFileInputStream(checksumFile))
- while (checksums.size < blockNum) {
- checksums += in.readLong()
+ Utils.tryWithResource {
+ new DataInputStream(new NioBufferedFileInputStream(checksumFile))
+ } { in =>
+ val checksums = new ArrayBuffer[Long]
+ while (checksums.size < blockNum) {
+ checksums += in.readLong()
+ }
+ checksums.toArray
}
} catch {
case _: IOException | _: EOFException =>
- return null
- } finally {
- in.close()
+ null
}
-
- checksums.toArray
}
/**
diff --git
a/core/src/test/scala/org/apache/spark/shuffle/sort/IndexShuffleBlockResolverSuite.scala
b/core/src/test/scala/org/apache/spark/shuffle/sort/IndexShuffleBlockResolverSuite.scala
index d374b54c8cb9..32eca21e96d7 100644
---
a/core/src/test/scala/org/apache/spark/shuffle/sort/IndexShuffleBlockResolverSuite.scala
+++
b/core/src/test/scala/org/apache/spark/shuffle/sort/IndexShuffleBlockResolverSuite.scala
@@ -278,6 +278,22 @@ class IndexShuffleBlockResolverSuite extends SparkFunSuite
{
val checksumsFromFile = resolver.getChecksums(checksumFile, 10)
assert(checksumsInMemory === checksumsFromFile)
}
+
+ test("SPARK-57504: getChecksums returns null instead of throwing NPE when
the " +
+ "checksum file cannot be opened") {
+ val resolver = new IndexShuffleBlockResolver(conf, blockManager)
+ // If opening the checksum file fails, getChecksums is meant to return
null: it
+ // already catches IOException/EOFException. But the old non-null-safe
+ // `finally { in.close() }` threw NPE when the failure came from the stream
+ // constructor (`in` was still null), masking the original error. The
sibling
+ // methods checkIndexAndDataFile and getMergedBlockData already handle this
+ // correctly. Force a constructor failure with a file that appears to
exist but
+ // cannot be opened.
+ val unopenableChecksumFile = new File(tempDir, "missing.checksum") {
+ override def exists(): Boolean = true
+ }
+ assert(resolver.getChecksums(unopenableChecksumFile, 10) === null)
+ }
}
class SslIndexShuffleBlockResolverSuite extends IndexShuffleBlockResolverSuite
{
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
