Lower flock LOG(FATAL) to LOG(WARNING)

Failing to unlock a file should not be a fatal error.

The lock will usually be cleared right after this anyway by the file
being closed. Even if it isn't though this should not cause deadlocks
since most places either don't retry if they don't get the lock the
first time or are separate binaries (such as dex2oat) that are run
with external timeouts.

Bug: 36369345
Test: ./test.py --host -j40
Change-Id: Icd783c038de3c263805e8de1bbc35b2ee8918340
(cherry picked from commit 9c48ee5bac5ef0d0af9c1ab079452c954affa019)

diff --git a/runtime/base/scoped_flock.cc b/runtime/base/scoped_flock.cc
index af57329..862f0d0 100644 (file)
--- a/runtime/base/scoped_flock.cc
+++ b/runtime/base/scoped_flock.cc
@@ -129,8 +129,12 @@ ScopedFlock::~ScopedFlock() {
   if (file_.get() != nullptr) {
     int flock_result = TEMP_FAILURE_RETRY(flock(file_->Fd(), LOCK_UN));
     if (flock_result != 0) {
-      PLOG(FATAL) << "Unable to unlock file " << file_->GetPath();
-      UNREACHABLE();
+      // Only printing a warning is okay since this is only used with either:
+      // 1) a non-blocking Init call, or
+      // 2) as a part of a seperate binary (eg dex2oat) which has it's own timeout logic to prevent
+      //    deadlocks.
+      // This means we can be sure that the warning won't cause a deadlock.
+      PLOG(WARNING) << "Unable to unlock file " << file_->GetPath();
     }
     int close_result = -1;
     if (file_->ReadOnlyMode() || !flush_on_close_) {

diff --git a/runtime/base/scoped_flock.h b/runtime/base/scoped_flock.h
index 7196b02..a3a320f 100644 (file)
--- a/runtime/base/scoped_flock.h
+++ b/runtime/base/scoped_flock.h
@@ -25,6 +25,15 @@
 
 namespace art {
 
+// A scoped file-lock implemented using flock. The file is locked by calling the Init function and
+// is released during destruction. Note that failing to unlock the file only causes a warning to be
+// printed. Users should take care that this does not cause potential deadlocks.
+//
+// Only printing a warning on unlock failure is okay since this is only used with either:
+// 1) a non-blocking Init call, or
+// 2) as a part of a seperate binary (eg dex2oat) which has it's own timeout logic to prevent
+//    deadlocks.
+// This means we can be sure that the warning won't cause a deadlock.
 class ScopedFlock {
  public:
   ScopedFlock();