PARQUET-537: Ensure that LocalFileSource is properly closed.

Author: Aliaksei Sandryhaila <aliaksei.sandryhaila@hp.com> Closes apache#68 from asandryh/PARQUET-537 and squashes the following commits: 18dca87 [Aliaksei Sandryhaila] Added a unit test. dfb7a0b [Aliaksei Sandryhaila] PARQUET-537: Ensure that LocalFileSource is properly closed.
wesm · Sep 2, 2018 · 938b843 · 938b843
1 parent ecdfad5
commit 938b843
Show file tree

Hide file tree

Showing 4 changed files with 45 additions and 3 deletions.
diff --git a/cpp/src/parquet/file/reader-internal.cc b/cpp/src/parquet/file/reader-internal.cc
@@ -215,6 +215,10 @@ void SerializedFile::Close() {
   source_->Close();
 }
 
+SerializedFile::~SerializedFile() {
+  Close();
+}
+
 std::shared_ptr<RowGroupReader> SerializedFile::GetRowGroup(int i) {
   std::unique_ptr<SerializedRowGroup> contents(new SerializedRowGroup(source_.get(),
           &metadata_.row_groups[i]));

diff --git a/cpp/src/parquet/file/reader-internal.h b/cpp/src/parquet/file/reader-internal.h
@@ -100,6 +100,7 @@ class SerializedFile : public ParquetFileReader::Contents {
   virtual int64_t num_rows() const;
   virtual int num_columns() const;
   virtual int num_row_groups() const;
+  virtual ~SerializedFile();
 
  private:
   // This class takes ownership of the provided data source

diff --git a/cpp/src/parquet/file/reader.cc b/cpp/src/parquet/file/reader.cc
@@ -65,7 +65,9 @@ RowGroupStatistics RowGroupReader::GetColumnStats(int i) const {
 // ParquetFileReader public API
 
 ParquetFileReader::ParquetFileReader() : schema_(nullptr) {}
-ParquetFileReader::~ParquetFileReader() {}
+ParquetFileReader::~ParquetFileReader() {
+  Close();
+}
 
 std::unique_ptr<ParquetFileReader> ParquetFileReader::OpenFile(const std::string& path,
     bool memory_map) {
@@ -91,7 +93,9 @@ void ParquetFileReader::Open(std::unique_ptr<ParquetFileReader::Contents> conten
 }
 
 void ParquetFileReader::Close() {
-  contents_->Close();
+  if (contents_) {
+    contents_->Close();
+  }
 }
 
 int ParquetFileReader::num_row_groups() const {

diff --git a/cpp/src/parquet/reader-test.cc b/cpp/src/parquet/reader-test.cc
@@ -16,15 +16,18 @@
 // under the License.
 
 #include <gtest/gtest.h>
+#include <fcntl.h>
 #include <cstdlib>
 #include <cstdint>
 #include <iostream>
 #include <memory>
 #include <string>
 
 #include "parquet/file/reader.h"
+#include "parquet/file/reader-internal.h"
 #include "parquet/column/reader.h"
 #include "parquet/column/scanner.h"
+#include "parquet/util/input.h"
 
 using std::string;
 
@@ -50,7 +53,6 @@ class TestAllTypesPlain : public ::testing::Test {
   std::unique_ptr<ParquetFileReader> reader_;
 };
 
-
 TEST_F(TestAllTypesPlain, NoopConstructDestruct) {
 }
 
@@ -121,4 +123,35 @@ TEST_F(TestAllTypesPlain, DebugPrintWorks) {
   ASSERT_GT(result.size(), 0);
 }
 
+
+class TestLocalFileSource : public ::testing::Test {
+ public:
+  void SetUp() {
+    std::string dir_string(data_dir);
+
+    std::stringstream ss;
+    ss << dir_string << "/" << "alltypes_plain.parquet";
+
+    file.reset(new LocalFileSource());
+    file->Open(ss.str());
+  }
+
+  void TearDown() {}
+
+ protected:
+  std::unique_ptr<LocalFileSource> file;
+};
+
+TEST_F(TestLocalFileSource, FileClosedOnDestruction) {
+  int file_desc = file->file_descriptor();
+  {
+    auto contents = SerializedFile::Open(std::move(file));
+    std::unique_ptr<ParquetFileReader> result(new ParquetFileReader());
+    result->Open(std::move(contents));
+  }
+  ASSERT_EQ(-1, fcntl(file_desc, F_GETFD));
+  ASSERT_EQ(EBADF, errno);
+}
+
+
 } // namespace parquet_cpp