Skip to content

[SPARK-50716][CORE][FOLLOWUP] Fix the scenario in JavaUtils#deleteRecursivelyUsingJavaIO where BasicFileAttributes cannot be read. #49357

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
LuciferYang wants to merge 3 commits into from
Closed

[SPARK-50716][CORE][FOLLOWUP] Fix the scenario in JavaUtils#deleteRecursivelyUsingJavaIO where BasicFileAttributes cannot be read. #49357

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
253c4c4
fix and test
LuciferYang Jan 5, 2025
43c6eb8
test macos pyspark
LuciferYang Jan 6, 2025
b6791cd
revert macos check
LuciferYang Jan 6, 2025
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
20 changes: 16 additions & 4 deletions common/utils/src/main/java/org/apache/spark/network/util/JavaUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,10 +126,10 @@ public static void deleteRecursively(File file, FilenameFilter filter) throws IO
private static void deleteRecursivelyUsingJavaIO(
File file,
FilenameFilter filter) throws IOException {
BasicFileAttributes fileAttributes =
Files.readAttributes(file.toPath(), BasicFileAttributes.class, LinkOption.NOFOLLOW_LINKS);
// SPARK-50716: If the file does not exist and not a broken symbolic link, return directly.
if (!file.exists() && !fileAttributes.isSymbolicLink()) return;
BasicFileAttributes fileAttributes = readFileAttributes(file);
// SPARK-50716: If the file attributes are null, that is, the file attributes cannot be read,
// or if the file does not exist and is not a broken symbolic link, then return directly.
if (fileAttributes == null || (!file.exists() && !fileAttributes.isSymbolicLink())) return;
if (fileAttributes.isDirectory()) {
IOException savedIOException = null;
for (File child : listFilesSafely(file, filter)) {
Expand All @@ -156,6 +156,18 @@ private static void deleteRecursivelyUsingJavaIO(
}
}

/**
* Reads basic attributes of a given file, of return null if an I/O error occurs.
*/
private static BasicFileAttributes readFileAttributes(File file) {
try {
return Files.readAttributes(
Copy link
Contributor Author

@LuciferYang LuciferYang Jan 5, 2025

Choose a reason for hiding this comment

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

cc @dongjoon-hyun

  1. If the inode itself does not exist, it will not be possible to read the BasicFileAttributes.
  2. If it is a corrupted symbolic link, the BasicFileAttributes can be read, but file.exists() will return false.

It seems that scenario 1 was not covered before. I found that the code has logic that attempts to delete a potentially existing target before writing.

For example:

spark/core/src/main/scala/org/apache/spark/SparkContext.scala

Lines 1879 to 1888 in 91f3fdd

val dest = new File(
root,
if (uri.getFragment != null) uri.getFragment else source.getName)
logInfo(
log"Unpacking an archive ${MDC(LogKeys.PATH, path)}" +
log" (${MDC(LogKeys.BYTE_SIZE, source.length)} bytes)" +
log" from ${MDC(LogKeys.SOURCE_PATH, source.getAbsolutePath)}" +
log" to ${MDC(LogKeys.DESTINATION_PATH, dest.getAbsolutePath)}")
Utils.deleteRecursively(dest)
Utils.unpack(source, dest)

Let's test it on macOS first.

Copy link
Contributor Author

@LuciferYang LuciferYang Jan 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It's getting a bit late in my time zone, so I need to sleep for 4 hours first.

Let's check the new test results and decide on the next steps. Of course, if you think the risk is too high, we can also revert this patch first. @dongjoon-hyun Thanks ~

Copy link
Member

@dongjoon-hyun dongjoon-hyun Jan 5, 2025

Choose a reason for hiding this comment

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

We can wait :)

file.toPath(), BasicFileAttributes.class, LinkOption.NOFOLLOW_LINKS);
} catch (IOException e) {
return null;
}
}

private static void deleteRecursivelyUsingUnixNative(File file) throws IOException {
ProcessBuilder builder = new ProcessBuilder("rm", "-rf", file.getAbsolutePath());
Process process = null;
Expand Down
Loading