arrow git commit: ARROW-455: [C++] Add dtor to BufferOutputStream that calls Close()

[we...@apache.org]

Repository: arrow
Updated Branches:
  refs/heads/master 9513ca774 -> 320f5875e


ARROW-455: [C++] Add dtor to BufferOutputStream that calls Close()

Since `Close()` can technically fail, it's better to call it yourself (and it's idempotent), but this will help avoid a common class of bugs in small-scale use cases.

An alternative here is that we could remove all `Close()` calls from all destructors and possibly add a `DCHECK(!is_open_)` to the base dtor to force the user to close handles. The downside of this is that it makes RAII more difficult, so I'd prefer to leave the close-in-dtor even though it can fail in unusual scenarios.

Author: Wes McKinney <we...@twosigma.com>

Closes #269 from wesm/ARROW-455 and squashes the following commits:

821ee22 [Wes McKinney] Add dtor to BufferOutputStream that calls Close()


Project: http://git-wip-us.apache.org/repos/asf/arrow/repo
Commit: http://git-wip-us.apache.org/repos/asf/arrow/commit/320f5875
Tree: http://git-wip-us.apache.org/repos/asf/arrow/tree/320f5875
Diff: http://git-wip-us.apache.org/repos/asf/arrow/diff/320f5875

Branch: refs/heads/master
Commit: 320f5875eef4010762a2146a0691148af1a3f182
Parents: 9513ca7
Author: Wes McKinney <we...@twosigma.com>
Authored: Thu Jan 5 15:18:24 2017 -0500
Committer: Wes McKinney <we...@twosigma.com>
Committed: Thu Jan 5 15:18:24 2017 -0500

----------------------------------------------------------------------
 cpp/src/arrow/io/file.cc           |  1 +
 cpp/src/arrow/io/io-memory-test.cc | 15 +++++++++++++--
 cpp/src/arrow/io/memory.cc         |  5 +++++
 cpp/src/arrow/io/memory.h          |  2 ++
 4 files changed, 21 insertions(+), 2 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/arrow/blob/320f5875/cpp/src/arrow/io/file.cc
----------------------------------------------------------------------
diff --git a/cpp/src/arrow/io/file.cc b/cpp/src/arrow/io/file.cc
index 3182f2d..0fb13ea 100644
--- a/cpp/src/arrow/io/file.cc
+++ b/cpp/src/arrow/io/file.cc
@@ -476,6 +476,7 @@ FileOutputStream::FileOutputStream() {
 }
 
 FileOutputStream::~FileOutputStream() {
+  // This can fail; better to explicitly call close
   impl_->Close();
 }
 

http://git-wip-us.apache.org/repos/asf/arrow/blob/320f5875/cpp/src/arrow/io/io-memory-test.cc
----------------------------------------------------------------------
diff --git a/cpp/src/arrow/io/io-memory-test.cc b/cpp/src/arrow/io/io-memory-test.cc
index 95d788c..c0b0165 100644
--- a/cpp/src/arrow/io/io-memory-test.cc
+++ b/cpp/src/arrow/io/io-memory-test.cc
@@ -42,17 +42,28 @@ class TestBufferOutputStream : public ::testing::Test {
   std::unique_ptr<OutputStream> stream_;
 };
 
+TEST_F(TestBufferOutputStream, DtorCloses) {
+  std::string data = "data123456";
+
+  const int K = 100;
+  for (int i = 0; i < K; ++i) {
+    EXPECT_OK(stream_->Write(data));
+  }
+
+  stream_ = nullptr;
+  ASSERT_EQ(static_cast<int64_t>(K * data.size()), buffer_->size());
+}
+
 TEST_F(TestBufferOutputStream, CloseResizes) {
   std::string data = "data123456";
 
-  const int64_t nbytes = static_cast<int64_t>(data.size());
   const int K = 100;
   for (int i = 0; i < K; ++i) {
     EXPECT_OK(stream_->Write(data));
   }
 
   ASSERT_OK(stream_->Close());
-  ASSERT_EQ(K * nbytes, buffer_->size());
+  ASSERT_EQ(static_cast<int64_t>(K * data.size()), buffer_->size());
 }
 
 TEST(TestBufferReader, RetainParentReference) {

http://git-wip-us.apache.org/repos/asf/arrow/blob/320f5875/cpp/src/arrow/io/memory.cc
----------------------------------------------------------------------
diff --git a/cpp/src/arrow/io/memory.cc b/cpp/src/arrow/io/memory.cc
index 4595268..0f5a0dc 100644
--- a/cpp/src/arrow/io/memory.cc
+++ b/cpp/src/arrow/io/memory.cc
@@ -43,6 +43,11 @@ BufferOutputStream::BufferOutputStream(const std::shared_ptr<ResizableBuffer>& b
       position_(0),
       mutable_data_(buffer->mutable_data()) {}
 
+BufferOutputStream::~BufferOutputStream() {
+  // This can fail, better to explicitly call close
+  Close();
+}
+
 Status BufferOutputStream::Close() {
   if (position_ < capacity_) {
     return buffer_->Resize(position_);

http://git-wip-us.apache.org/repos/asf/arrow/blob/320f5875/cpp/src/arrow/io/memory.h
----------------------------------------------------------------------
diff --git a/cpp/src/arrow/io/memory.h b/cpp/src/arrow/io/memory.h
index 2f1d8ec..8428a12 100644
--- a/cpp/src/arrow/io/memory.h
+++ b/cpp/src/arrow/io/memory.h
@@ -43,6 +43,8 @@ class ARROW_EXPORT BufferOutputStream : public OutputStream {
  public:
   explicit BufferOutputStream(const std::shared_ptr<ResizableBuffer>& buffer);
 
+  ~BufferOutputStream();
+
   // Implement the OutputStream interface
   Status Close() override;
   Status Tell(int64_t* position) override;