Patchwork blackbox: defer opening a log file until needed (issue3869)

login
register
mail settings
Submitter Bryan O'Sullivan
Date March 26, 2013, 11:28 p.m.
Message ID <b399b3dfd25471dfb998.1364340504@australite.local>
Download mbox | patch
Permalink /patch/1199/
State Accepted
Commit 17f6644a2fbca63c37880ffbd8bc43e05a14837f
Headers show

Comments

Bryan O'Sullivan - March 26, 2013, 11:28 p.m.
# HG changeset patch
# User Bryan O'Sullivan <bryano@fb.com>
# Date 1364340471 25200
#      Tue Mar 26 16:27:51 2013 -0700
# Node ID b399b3dfd25471dfb99856419e90d49961c85c58
# Parent  b2a36e9b9ccc9b6187efa93970eac7547aeb9632
blackbox: defer opening a log file until needed (issue3869)

Previously, we opened the log file when creating a repo object. This
was inefficient (not all repo creation is going to result in a need to
log something), but more importantly it broke subrepo updates when used
on NFS.

* perform an update in the master repo that triggers a subrepo clone

* empty subrepo already exists, and has an open, empty blackbox.log file
  due to it being opened eagerly/prematurely

* hg decides to blow away the skeletal subrepo (see use of shutil.rmtree
  in subrepo._get)

* we crash, due to NFS treating a delete of an open file as really a
  rename to a hidden ".nfs" file

Now that we open the blackbox log file on demand, no file exists at the
time the empty subrepo is deleted, so the above problem does not occur.
Durham Goode - March 26, 2013, 11:50 p.m.
On 3/26/13 4:28 PM, "Bryan O'Sullivan" <bos@serpentine.com> wrote:

># HG changeset patch
># User Bryan O'Sullivan <bryano@fb.com>
># Date 1364340471 25200
>#      Tue Mar 26 16:27:51 2013 -0700
># Node ID b399b3dfd25471dfb99856419e90d49961c85c58
># Parent  b2a36e9b9ccc9b6187efa93970eac7547aeb9632
>blackbox: defer opening a log file until needed (issue3869)
>

LGTM.  Should we close the file after every write as well so we don't
encounter any other cases where NFS can't delete the tree while it's open?
Bryan O'Sullivan - March 27, 2013, 12:17 a.m.
On Tue, Mar 26, 2013 at 4:50 PM, Durham Goode <durham@fb.com> wrote:

> LGTM.  Should we close the file after every write as well so we don't
> encounter any other cases where NFS can't delete the tree while it's open?
>

Maybe, but I haven't seen any such cases arise. I think this could maybe be
provoked by replacing a subrepo that points at one URL with another, but I
haven't figured out how, and absent a way to reproduce such a hypothetical
error, I'm not sure I'm bothered.

Patch

diff --git a/hgext/blackbox.py b/hgext/blackbox.py
--- a/hgext/blackbox.py
+++ b/hgext/blackbox.py
@@ -47,6 +47,15 @@  def wrapui(ui):
 
             if util.safehasattr(self, '_blackbox'):
                 blackbox = self._blackbox
+            elif util.safehasattr(self, '_bbopener'):
+                try:
+                    self._blackbox = self._bbopener('blackbox.log', 'a')
+                except (IOError, OSError), err:
+                    self.debug('warning: cannot write to blackbox.log: %s\n' %
+                               err.strerror)
+                    del self._bbopener
+                    self._blackbox = None
+                blackbox = self._blackbox
             else:
                 # certain ui instances exist outside the context of
                 # a repo, so just default to the last blackbox that
@@ -65,12 +74,7 @@  def wrapui(ui):
                 lastblackbox = blackbox
 
         def setrepo(self, repo):
-            try:
-                self._blackbox = repo.opener('blackbox.log', 'a')
-            except (IOError, OSError), err:
-                self.debug('warning: cannot write to blackbox.log: %s\n' %
-                           err.strerror)
-                self._blackbox = None
+            self._bbopener = repo.opener
 
     ui.__class__ = blackboxui