From 938b843213c4552f8f1a0fe6d2debde3bbc3b340 Mon Sep 17 00:00:00 2001
From: Aliaksei Sandryhaila <aliaksei.sandryhaila@hp.com>
Date: Tue, 1 Mar 2016 17:33:51 -0800
Subject: [PATCH] PARQUET-537: Ensure that LocalFileSource is properly closed.

Author: Aliaksei Sandryhaila <aliaksei.sandryhaila@hp.com>

Closes #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.
---
 cpp/src/parquet/file/reader-internal.cc |  4 +++
 cpp/src/parquet/file/reader-internal.h  |  1 +
 cpp/src/parquet/file/reader.cc          |  8 ++++--
 cpp/src/parquet/reader-test.cc          | 35 ++++++++++++++++++++++++-
 4 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/cpp/src/parquet/file/reader-internal.cc b/cpp/src/parquet/file/reader-internal.cc
index 3d8c373f97833..c571c72e434a4 100644
--- 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
index 08d4607540137..a398cb332997a 100644
--- 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
index fcbe4530fcbf2..8f639e9a1e42b 100644
--- 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
index 3ac1525edb800..2c69ce12a1e43 100644
--- a/cpp/src/parquet/reader-test.cc
+++ b/cpp/src/parquet/reader-test.cc
@@ -16,6 +16,7 @@
 // under the License.
 
 #include <gtest/gtest.h>
+#include <fcntl.h>
 #include <cstdlib>
 #include <cstdint>
 #include <iostream>
@@ -23,8 +24,10 @@
 #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