Skip to content
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.

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

Mask getBasicFileAttributes errors in Files[F].walk #3120

Merged
merged 3 commits into from
Feb 4, 2023

Conversation

fthomas
Copy link
Member

@fthomas fthomas commented Jan 31, 2023

This shows a problem with Files[F].walk where it does not walk the whole file tree if files are deleted during the process. The added tests fails with:

==> X fs2.io.file.FilesSuite.walk - can delete files in a nested tree  0.029s munit.ComparisonFailException: fs2/io/shared/src/test/scala/fs2/io/file/FilesSuite.scala:537
536:        .foldMonoid
537:        .assertEquals(25)
538:    }
values are not the same
=> Obtained
5
=> Diff (- obtained, + expected)
-5
+25

Only 5 files out of 25 where deleted. I expected that the whole process deletes 25 files because tempFilesHierarchy creates 5 child directories with 5 files in each child directory.

If Files[IO].deleteIfExists(path).as(1) is replaced with IO.pure(1) the test passes.

@armanbilge
Copy link
Member

Thanks for the reproducer! So the issue is partly related to this .mask, which swallows the exception and stops iterating over the directory.

if (attr.isDirectory)
list(start).flatMap { path =>
go(path, maxDepth - 1, attr.fileKey.toRight(start) :: ancestry)
}.mask

If I take it away I get this.

==> X fs2.io.file.FilesSuite.walk - can delete files in a nested tree  0.482s java.nio.file.NoSuchFileException: /tmp/BaseFileSpec9152885937877967676/BaseFileSpec10790971575620310752/BaseFileSpecSub14584697728072684076.tmp

@armanbilge
Copy link
Member

Ah yes, more masking seems to have solved it 😂

@armanbilge armanbilge changed the title Files[F].walk cannot delete files recursively Mask getBasicFileAttributes errors in Files[F].walk Jan 31, 2023
@armanbilge armanbilge changed the title Mask getBasicFileAttributes errors in Files[F].walk Mask getBasicFileAttributes errors in Files[F].walk Jan 31, 2023
Comment on lines +477 to +492
Stream.eval(getBasicFileAttributes(start, followLinks = true)).mask.flatMap {
attr =>
val fileKey = attr.fileKey
val isCycle = Traverse[List].existsM(ancestry) {
case Right(ancestorKey) => F.pure(fileKey.contains(ancestorKey))
case Left(ancestorPath) => isSameFile(start, ancestorPath)
}

Stream.eval(isCycle).flatMap { isCycle =>
if (!isCycle)
list(start).mask.flatMap { path =>
go(path, maxDepth - 1, attr.fileKey.toRight(start) :: ancestry)
}
else
Stream.raiseError(new FileSystemLoopException(start.toString))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if some of this could be contained within the F effect itself, as there is no much use for streaming features:

Suggested change
Stream.eval(getBasicFileAttributes(start, followLinks = true)).mask.flatMap {
attr =>
val fileKey = attr.fileKey
val isCycle = Traverse[List].existsM(ancestry) {
case Right(ancestorKey) => F.pure(fileKey.contains(ancestorKey))
case Left(ancestorPath) => isSameFile(start, ancestorPath)
}
Stream.eval(isCycle).flatMap { isCycle =>
if (!isCycle)
list(start).mask.flatMap { path =>
go(path, maxDepth - 1, attr.fileKey.toRight(start) :: ancestry)
}
else
Stream.raiseError(new FileSystemLoopException(start.toString))
}
getBasicFileAttributes(start, followLinks = true).handleError(_ => ()).flatMap {
attr =>
val fileKey = attr.fileKey
val isCycle = Traverse[List].existsM(ancestry) {
case Right(ancestorKey) => F.pure(fileKey.contains(ancestorKey))
case Left(ancestorPath) => isSameFile(start, ancestorPath)
}
isCycle.flatMap {
case false =>
list(start).mask.flatMap { path =>
go(path, maxDepth - 1, attr.fileKey.toRight(start) :: ancestry)
}
case true => F.raiseError(new FileSystemLoopException(start.toString))
}

Copy link
Member Author

Choose a reason for hiding this comment

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

go is a Stream[F, Path] and you flatMap from F to it. I don't think this compiles.

@mpilquist mpilquist merged commit dddd44a into typelevel:main Feb 4, 2023
@fthomas fthomas deleted the topic/delete-files-with-walk branch February 4, 2023 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants