Skip to content

HDFS-16031. Possible Resource Leak in org.apache.hadoop.hdfs.server.aliasmap#InMemoryAliasMap #3027

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

Merged
aajisaka merged 3 commits into from
May 26, 2021
Merged

HDFS-16031. Possible Resource Leak in org.apache.hadoop.hdfs.server.aliasmap#InMemoryAliasMap #3027

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
920d339
possible resource leak in InMemoryAliasMap
Nargeshdb May 19, 2021
d0d81d6
use try-with-resources
Nargeshdb May 23, 2021
6a74dd9
remove tOut.finish()
Nargeshdb May 24, 2021
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
17 changes: 7 additions & 10 deletions ...ct/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/aliasmap/InMemoryAliasMap.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,21 +320,18 @@ static File createSnapshot(InMemoryAliasMap aliasMap) throws IOException {
private static File getCompressedAliasMap(File aliasMapDir)
throws IOException {
File outCompressedFile = new File(aliasMapDir.getParent(), TAR_NAME);
BufferedOutputStream bOut = null;
GzipCompressorOutputStream gzOut = null;
TarArchiveOutputStream tOut = null;
try {
bOut = new BufferedOutputStream(
Files.newOutputStream(outCompressedFile.toPath()));
gzOut = new GzipCompressorOutputStream(bOut);
tOut = new TarArchiveOutputStream(gzOut);

try (BufferedOutputStream bOut = new BufferedOutputStream(
Files.newOutputStream(outCompressedFile.toPath()));
GzipCompressorOutputStream gzOut = new GzipCompressorOutputStream(bOut);
TarArchiveOutputStream tOut = new TarArchiveOutputStream(gzOut)){

addFileToTarGzRecursively(tOut, aliasMapDir, "", new Configuration());
} finally {
if (tOut != null) {
tOut.finish();
}
Copy link
Member

@aajisaka aajisaka May 24, 2021

Choose a reason for hiding this comment

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

Thanks for the update.

  • Before: tOut.finish() is called if addFileToTarGzRecursively throws an exception.
  • Your patch: tOut.finish() is not called if addFileToTarGzRecursively throws an exception.

I think we need to have an extra try-catch clause:

      try {
        addFileToTarGzRecursively(tOut, aliasMapDir, "", new Configuration());
      } finally {
        tOut.finish();
      }

tOut cannot be null in the try-with-resources clause so that we can remove the null check.

Copy link
Contributor Author

@Nargeshdb Nargeshdb May 24, 2021

Choose a reason for hiding this comment

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

If we define tOut in try-with-resources block, then we don't have access to tOut in finally block. I checked close() method in TarArchiveOutputStream and finish() is called there so I think we don't need to call finish on tOut.

Copy link
Contributor Author

@Nargeshdb Nargeshdb May 24, 2021

Choose a reason for hiding this comment

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

6a74dd9
let me know if there is any problem.

IOUtils.cleanupWithLogger(null, tOut, gzOut, bOut);
}

return outCompressedFile;
}

Expand Down