Mailing-List: contact commits-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@impala.incubator.apache.org
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
From: tarmstrong@apache.org
To: commits@impala.incubator.apache.org
Date: Thu, 17 Nov 2016 16:09:28 -0000
Message-Id: <d6e89275477f4b2aa969d38229496271@git.apache.org>
In-Reply-To: <36bd3629b3874cbfb622f1fa4b2f77c0@git.apache.org>
References: <36bd3629b3874cbfb622f1fa4b2f77c0@git.apache.org>
Subject: [17/50] [abbrv] incubator-impala git commit: IMPALA-4437: fix crash
 in disk-io-mgr
archived-at: Thu, 17 Nov 2016 16:09:21 -0000

IMPALA-4437: fix crash in disk-io-mgr

This fixes another issue where the 'buffer_' field was not set to NULL
on an error, triggering a DCHECK.

Testing:
Added a unit test that triggers the bug on the two different codepaths
that I fixed.

Change-Id: Ib76cf5ba8d368b2b37bdc1d2133b8ddcb39f9e00
Reviewed-on: http://gerrit.cloudera.org:8080/4979
Reviewed-by: Dan Hecht <dhecht@cloudera.com>
Tested-by: Internal Jenkins


Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/ef689edf
Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/ef689edf
Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/ef689edf

Branch: refs/heads/hadoop-next
Commit: ef689edf36dd5715f17438e07a16fd4e38af5fb9
Parents: 51b1310
Author: Tim Armstrong <tarmstrong@cloudera.com>
Authored: Mon Nov 7 14:38:07 2016 -0800
Committer: Internal Jenkins <cloudera-hudson@gerrit.cloudera.org>
Committed: Tue Nov 8 08:29:58 2016 +0000

----------------------------------------------------------------------
 be/src/runtime/disk-io-mgr-test.cc | 38 +++++++++++++++++++++++++++++++++
 be/src/runtime/disk-io-mgr.cc      |  2 ++
 2 files changed, 40 insertions(+)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/ef689edf/be/src/runtime/disk-io-mgr-test.cc
----------------------------------------------------------------------
diff --git a/be/src/runtime/disk-io-mgr-test.cc b/be/src/runtime/disk-io-mgr-test.cc
index ef7e12f..9f8d6d7 100644
--- a/be/src/runtime/disk-io-mgr-test.cc
+++ b/be/src/runtime/disk-io-mgr-test.cc
@@ -1090,6 +1090,44 @@ TEST_F(DiskIoMgrTest, ReadIntoClientBuffer) {
   io_mgr.reset();
   EXPECT_EQ(mem_tracker.consumption(), 0);
 }
+
+// Test reading into a client-allocated buffer where the read fails.
+TEST_F(DiskIoMgrTest, ReadIntoClientBufferError) {
+  MemTracker mem_tracker(LARGE_MEM_LIMIT);
+  const char* tmp_file = "/file/that/does/not/exist";
+  const int SCAN_LEN = 128;
+
+  scoped_ptr<DiskIoMgr> io_mgr(new DiskIoMgr(1, 1, SCAN_LEN, SCAN_LEN));
+
+  ASSERT_OK(io_mgr->Init(&mem_tracker));
+  // Reader doesn't need to provide mem tracker if it's providing buffers.
+  MemTracker* reader_mem_tracker = NULL;
+  DiskIoRequestContext* reader;
+  vector<uint8_t> client_buffer(SCAN_LEN);
+  for (int i = 0; i < 1000; ++i) {
+    ASSERT_OK(io_mgr->RegisterContext(&reader, reader_mem_tracker));
+    DiskIoMgr::ScanRange* range = AllocateRange(1);
+    range->Reset(NULL, tmp_file, SCAN_LEN, 0, 0, true,
+        DiskIoMgr::BufferOpts::ReadInto(&client_buffer[0], SCAN_LEN));
+    ASSERT_OK(io_mgr->AddScanRange(reader, range, true));
+
+    /// Also test the cancellation path. Run multiple iterations since it is racy whether
+    /// the read fails before the cancellation.
+    if (i >= 1) io_mgr->CancelContext(reader);
+
+    DiskIoMgr::BufferDescriptor* io_buffer;
+    ASSERT_FALSE(range->GetNext(&io_buffer).ok());
+
+    // DiskIoMgr should not have allocated memory.
+    EXPECT_EQ(mem_tracker.consumption(), 0);
+
+    io_mgr->UnregisterContext(reader);
+  }
+
+  pool_.reset();
+  io_mgr.reset();
+  EXPECT_EQ(mem_tracker.consumption(), 0);
+}
 }
 
 int main(int argc, char** argv) {

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/ef689edf/be/src/runtime/disk-io-mgr.cc
----------------------------------------------------------------------
diff --git a/be/src/runtime/disk-io-mgr.cc b/be/src/runtime/disk-io-mgr.cc
index 265b49a..87ac33a 100644
--- a/be/src/runtime/disk-io-mgr.cc
+++ b/be/src/runtime/disk-io-mgr.cc
@@ -981,6 +981,7 @@ void DiskIoMgr::HandleReadFinished(DiskQueue* disk_queue, DiskIoRequestContext*
     state.DecrementRequestThreadAndCheckDone(reader);
     DCHECK(reader->Validate()) << endl << reader->DebugString();
     if (!buffer->is_client_buffer()) FreeBufferMemory(buffer);
+    buffer->buffer_ = NULL;
     buffer->scan_range_->Cancel(reader->status_);
     // Enqueue the buffer to use the scan range's buffer cleanup path.
     buffer->scan_range_->EnqueueBuffer(buffer);
@@ -997,6 +998,7 @@ void DiskIoMgr::HandleReadFinished(DiskQueue* disk_queue, DiskIoRequestContext*
   if (!buffer->status_.ok()) {
     // Error case
     if (!buffer->is_client_buffer()) FreeBufferMemory(buffer);
+    buffer->buffer_ = NULL;
     buffer->eosr_ = true;
     --state.num_remaining_ranges();
     buffer->scan_range_->Cancel(buffer->status_);