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.

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

Fixed resource leak in sootup.java.bytecode.frontend module #1099

Merged
merged 6 commits into from
Oct 25, 2024
Merged

Fixed resource leak in sootup.java.bytecode.frontend module #1099

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8701d2e
Fixed resource leak with try with resource statement
Bryce7832 Oct 7, 2024
eb90cf6
Fixing resource leak
Bryce7832 Oct 7, 2024
4ddf0c3
Solved complication error
Bryce7832 Oct 7, 2024
09e0855
Merge branch 'develop' into bug/resourceleak_bytecode
swissiety Oct 10, 2024
a7bb4f8
refactor: getClassProvider + reuse Local
swissiety Oct 25, 2024
e52d729
Merge branch 'develop' into bug/resourceleak_bytecode
swissiety Oct 25, 2024
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
35 changes: 20 additions & 15 deletions .../java/sootup/java/bytecode/frontend/inputlocation/JrtFileSystemAnalysisInputLocation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import sootup.core.IdentifierFactory;
import sootup.core.frontend.ClassProvider;
import sootup.core.frontend.ResolveException;
import sootup.core.frontend.SootClassSource;
import sootup.core.inputlocation.AnalysisInputLocation;
import sootup.core.model.SourceType;
import sootup.core.transform.BodyInterceptor;
Expand Down Expand Up @@ -149,21 +150,25 @@ protected Stream<JavaSootClassSource> getClassSourcesInternal(
+ classProvider.getHandledFileType().getExtensionWithDot();

final Path archiveRoot = theFileSystem.getPath("modules", moduleSignature.getModuleName());
try {

return Files.walk(archiveRoot)
.filter(
filePath ->
!Files.isDirectory(filePath)
&& filePath
.toString()
.endsWith(classProvider.getHandledFileType().getExtensionWithDot())
&& !filePath.toString().endsWith(moduleInfoFilename))
.flatMap(
p ->
StreamUtils.optionalToStream(
classProvider.createClassSource(this, p, fromPath(p, identifierFactory))))
.map(src -> (JavaSootClassSource) src);
try (Stream<Path> paths = Files.walk(archiveRoot)) {
// collect into a list and then return a stream, so we do not leak the Stream returned by
// Files.walk
List<JavaSootClassSource> javaSootClassSources =
paths
.filter(
filePath ->
!Files.isDirectory(filePath)
&& filePath
.toString()
.endsWith(classProvider.getHandledFileType().getExtensionWithDot())
&& !filePath.toString().endsWith(moduleInfoFilename))
.<SootClassSource>flatMap(
p ->
StreamUtils.optionalToStream(
classProvider.createClassSource(this, p, fromPath(p, identifierFactory))))
.map(src -> (JavaSootClassSource) src)
.collect(Collectors.toList());
return javaSootClassSources.stream();
Comment on lines +173 to +174
Copy link
Collaborator

@swissiety swissiety Oct 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

please don' t .collect() and .stream() again (return stream directly)

Copy link

Choose a reason for hiding this comment

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

Hi @swissiety since streams are lazy, if we don't collect, we cannot safely close the Stream returned by Files.walk within this method (since the filter and map operations won't have been executed yet). The collect forces those operations to execute, so then the stream can be closed. If you prefer not to collect and then stream again, an alternative would be to require all callers of this method to close the returned stream. Would that be preferable?

Copy link
Collaborator

Choose a reason for hiding this comment

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

thx @msridhar for pointing to that situation! As this is a protected method and its calling contexts are collecting the Stream again i would prefer to close the Stream in the two calling method(s) due to have less allocations

Copy link

Choose a reason for hiding this comment

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

@Bryce7832 could you update the PR to get rid of the collect to a list, and instead use try-with-resources at both callers of this method?

} catch (IOException e) {
throw new ResolveException("Error loading module " + moduleSignature, archiveRoot, e);
}
Expand Down
Loading