Bug: `IllegalStateException` at `java.base/java.nio.file.FileTreeIterator.hasNext(FileTreeIterator.java:102)`
SimonMarquis opened this issue · 12 comments
Version 0.2 (downloaded from maven) crashes on Windows with the following stacktrace:
java -jar sort-gradle-dependencies-app-0.2-all.jar .
[main] INFO Sorter - Sorting build.gradle(.kts) scripts in the following paths: .
java.lang.IllegalStateException
at java.base/java.nio.file.FileTreeIterator.hasNext(FileTreeIterator.java:102)
at java.base/java.util.Spliterators$IteratorSpliterator.tryAdvance(Spliterators.java:1855)
at java.base/java.util.stream.StreamSpliterators$WrappingSpliterator.lambda$initPartialTraversalState$0(StreamSpliterators.java:292)
at java.base/java.util.stream.StreamSpliterators$AbstractWrappingSpliterator.fillBuffer(StreamSpliterators.java:206)
at java.base/java.util.stream.StreamSpliterators$AbstractWrappingSpliterator.doAdvance(StreamSpliterators.java:161)
at java.base/java.util.stream.StreamSpliterators$WrappingSpliterator.tryAdvance(StreamSpliterators.java:298)
at java.base/java.util.Spliterators$1Adapter.hasNext(Spliterators.java:681)
at kotlin.sequences.FlatteningSequence$iterator$1.ensureItemIterator(Sequences.kt:316)
at kotlin.sequences.FlatteningSequence$iterator$1.hasNext(Sequences.kt:303)
at kotlin.sequences.SequencesKt___SequencesKt.toCollection(_Sequences.kt:787)
at kotlin.sequences.SequencesKt___SequencesKt.toSet(_Sequences.kt:828)
at com.squareup.sort.BuildDotGradleFinder.<init>(BuildDotGradleFinder.kt:28)
at com.squareup.sort.BuildDotGradleFinder$Factory$DefaultImpls.of(BuildDotGradleFinder.kt:34)
at com.squareup.sort.SortCommand$1.of(SortCommand.kt:47)
at com.squareup.sort.SortCommand.call(SortCommand.kt:87)
at com.squareup.sort.SortCommand.call(SortCommand.kt:36)
at picocli.CommandLine.executeUserObject(CommandLine.java:1953)
at picocli.CommandLine.access$1300(CommandLine.java:145)
at picocli.CommandLine$RunLast.executeUserObjectOfLastSubcommandWithSameParent(CommandLine.java:2358)
at picocli.CommandLine$RunLast.handle(CommandLine.java:2352)
at picocli.CommandLine$RunLast.handle(CommandLine.java:2314)
at picocli.CommandLine$AbstractParseResultHandler.execute(CommandLine.java:2179)
at picocli.CommandLine$RunLast.execute(CommandLine.java:2316)
at picocli.CommandLine.execute(CommandLine.java:2078)
at com.squareup.sort.MainKt.main(main.kt:15)
java -version
openjdk version "17.0.6" 2023-01-17
OpenJDK Runtime Environment Temurin-17.0.6+10 (build 17.0.6+10)
OpenJDK 64-Bit Server VM Temurin-17.0.6+10 (build 17.0.6+10, mixed mode, sharing)
The corresponding line seems to refer to this code:
@Override
public boolean hasNext() {
if (!walker.isOpen())
throw new IllegalStateException();
fetchNextIfNeeded();
return next != null;
}This seems to be caused by this:
More specifically the fact that we use the .use { } extension, which closes the underlying file Stream before the terminal .toSet() is called.
A "simple" solution to this could be to use Kotlin's Path.walk() API.
It is listed under @ExperimentalPathApi, but works fine everywhere I tested it.
We loose the Stream.parallel() thing, although it does not seem to make a dramatic difference since we don't execute operations on each item, but rather only do filtering.
- Files.walk(searchPath).use { paths ->
- paths.parallel()
- .filter(Path::isBuildDotGradle)
- .asSequence()
- }
+ searchPath.walk().filter(Path::isBuildDotGradle)You could also use the non-experimental File.walk() in the stdlib to avoid the experimental API?
Yes for sure, this would need to be collected first, then transformed to a sequence like that:
Files.walk(searchPath).use { paths ->
paths.parallel()
.filter(Path::isBuildDotGradle)
.collect(Collectors.toList())
.asSequence()
}If that's good for you I can update the PR 👍
ah I think I wasn't clear, I meant dropping the Path APIs entirely like this
searchPath.toFile().walkTopDown()
.filter(File::isBuildDotGradle)
.toList()In what way would we benefit from dropping this API entirely? 🙃
I mean, most of this plugin relies on the Path API. Migrating to File would force us to modify a lot of method signatures. Also, I tought Paths were more "lightweight" than Files.
Or do you meant to re-map Files to Paths at the end of this method?
To be honest, I'm a bit confused now on what shape of API you want for this method. Could you give us more details?
In what way would we benefit from dropping this API entirely?
As stated above, to avoid using an experimental API. My previous comment is an example of that. I'm personally ok with using the experimental API, but I'd defer to @autonomousapps on that.
Ok, I get it. But this solution using Files.walk(Path) is not experimental.
What do you think of it? It would have the benefit of not changing the API signature while keeping stable APIs.
That solution's pretty inefficient because it collects to an intermediary list first when it could just be done in a single pass with Path.walk() + filter. Plus kotlin's file walk gives you some useful hooks to avoid entering known directories like build.
That makes sense. Then would it be ok to map it back to Path at the end to keep the same signature for buildDotGradles?
Quick remark about this line if (searchPath.isBuildDotGradle()) { sequenceOf(searchPath) }: this is already the behavior of File/Path walkers. Shouldn't we keep our code as simple as possible, and pay the "tiny" price of allocating those objects?
Also, if we want to be really efficient and skip directories like .git, .gradle, build, src etc., we would have to create our own FileVisitor or visitFileTree (which is experimental).
But then I guess this is out of the scope of this PR. Though I'd love to create another PR for this.
Edit: I just found out the hooks are to be used directly on the FileTreeWalk instance. I've created #49 for that.
@SimonMarquis circling back to this, you have two PRs open and I think we got a little lost in the weeds. Let's go with a simple approach for this, I'll put up a PR shortly