IMPALA-4946: fix hang in BufferPool Once the write is removed from the "in flight" list, both the Client and Page may be destroyed by a different thread. The fix is to signal condition variables before inside the critical section that removes the write from the in flight list.
Also fix a potential pitfall with DiskIoMgr::CancelContext() where concurrent calls to the method, which can be called asynchronously with other methods, could result in a hang in DiskIoMgr::CancelContext(). I do not believe any Impala code calls it concurrently from multiple threads, so the bug was only latent. Testing: I was able to reproduce reliably by inserting a 1s sleep before the NotifyAll() calls. After the fix, the hang didn't reproduce with sleeps inside or outside the critical section. I could not come up with a unit test that had a higher reproduction rate than the current tests - the window for the race is very small. I considered adding a debug stress option to insert these delays, but with all the code moved into the critical section it wouldn't be useful. Change-Id: I13fc95b5a664544dee789c4107fccf81d2077347 Reviewed-on: http://gerrit.cloudera.org:8080/6224 Reviewed-by: Dan Hecht <[email protected]> Tested-by: Impala Public 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/47f7de9e Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/47f7de9e Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/47f7de9e Branch: refs/heads/master Commit: 47f7de9e2d76d7100d9a8486f6ac8ee27bd7a368 Parents: 996fb5e Author: Tim Armstrong <[email protected]> Authored: Wed Mar 1 22:33:40 2017 -0800 Committer: Impala Public Jenkins <[email protected]> Committed: Fri Mar 3 03:47:42 2017 +0000 ---------------------------------------------------------------------- be/src/runtime/bufferpool/buffer-pool.cc | 6 ++++-- be/src/runtime/disk-io-mgr-internal.h | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/47f7de9e/be/src/runtime/bufferpool/buffer-pool.cc ---------------------------------------------------------------------- diff --git a/be/src/runtime/bufferpool/buffer-pool.cc b/be/src/runtime/bufferpool/buffer-pool.cc index 863effc..7a026d6 100644 --- a/be/src/runtime/bufferpool/buffer-pool.cc +++ b/be/src/runtime/bufferpool/buffer-pool.cc @@ -601,9 +601,11 @@ void BufferPool::Client::WriteCompleteCallback(Page* page, const Status& write_s dirty_unpinned_bytes_ -= page->len; in_flight_write_bytes_ -= page->len; WriteDirtyPagesAsync(); // Start another asynchronous write if needed. + + // Notify before releasing lock to avoid race with Page and Client destruction. + page->write_complete_cv_.NotifyAll(); + write_complete_cv_.NotifyAll(); } - write_complete_cv_.NotifyAll(); - page->write_complete_cv_.NotifyAll(); } void BufferPool::Client::WaitForWrite(unique_lock<mutex>* client_lock, Page* page) { http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/47f7de9e/be/src/runtime/disk-io-mgr-internal.h ---------------------------------------------------------------------- diff --git a/be/src/runtime/disk-io-mgr-internal.h b/be/src/runtime/disk-io-mgr-internal.h index 17b02af..3c5c5ba 100644 --- a/be/src/runtime/disk-io-mgr-internal.h +++ b/be/src/runtime/disk-io-mgr-internal.h @@ -157,7 +157,7 @@ class DiskIoRequestContext { // boost doesn't let us dcheck that the reader lock is taken DCHECK_GT(num_disks_with_ranges_, 0); if (--num_disks_with_ranges_ == 0) { - disks_complete_cond_var_.notify_one(); + disks_complete_cond_var_.notify_all(); } DCHECK(Validate()) << std::endl << DebugString(); }
