Patchwork [5,of,7] blackbox: avoid creating multiple file handles for a single log

login
register
mail settings
Submitter timeless@mozdev.org
Date Feb. 3, 2016, 9:40 p.m.
Message ID <ed58e3820ee3c6aa1500.1454535647@waste.org>
Download mbox | patch
Permalink /patch/12965/
State Superseded
Commit 45313f5a3a8c34ce02f6cb0863386eb0cea534ac
Delegated to: Yuya Nishihara
Headers show

Comments

timeless@mozdev.org - Feb. 3, 2016, 9:40 p.m.
# HG changeset patch
# User timeless <timeless@mozdev.org>
# Date 1454514091 0
#      Wed Feb 03 15:41:31 2016 +0000
# Node ID ed58e3820ee3c6aa1500cfbe92bf4d390df849e4
# Parent  7d55ab6425b5b8836560e0de40e20e47c762a0f9
blackbox: avoid creating multiple file handles for a single log

There are multiple ui objects in Mercurial that can relate to a repository,
before this change, each one would have its own file pointer, which
results in unfortunate logging behavior.

Also, any log rotation results would be bad because only the
active blackboxui object's file pointer would be refreshed.
Yuya Nishihara - Feb. 7, 2016, 2:28 p.m.
On Wed, 03 Feb 2016 15:40:47 -0600, timeless wrote:
> # HG changeset patch
> # User timeless <timeless@mozdev.org>
> # Date 1454514091 0
> #      Wed Feb 03 15:41:31 2016 +0000
> # Node ID ed58e3820ee3c6aa1500cfbe92bf4d390df849e4
> # Parent  7d55ab6425b5b8836560e0de40e20e47c762a0f9
> blackbox: avoid creating multiple file handles for a single log
> 
> There are multiple ui objects in Mercurial that can relate to a repository,
> before this change, each one would have its own file pointer, which
> results in unfortunate logging behavior.
> 
> Also, any log rotation results would be bad because only the
> active blackboxui object's file pointer would be refreshed.
> 
> diff --git a/hgext/blackbox.py b/hgext/blackbox.py
> --- a/hgext/blackbox.py
> +++ b/hgext/blackbox.py
> @@ -42,6 +42,22 @@
>  testedwith = 'internal'
>  lastblackbox = None
>  
> +filehandles = {}

Did you try overriding blackboxui.__init__ ? It should be called with "src"
ui when copying ui object. I think global cache is the last option.

>          def setrepo(self, repo):
> -            self._bbopener = repo.vfs
> +            self._bbvfs = repo.vfs

Please do bulk renaming by a separate patch.
timeless - Feb. 8, 2016, 1:32 a.m.
Yuya Nishihara wrote:
> Did you try overriding blackboxui.__init__ ? It should be called with "src"
> ui when copying ui object. I think global cache is the last option.

This code already has a global cache of sort for the `lastblackbox` file handle.

And yes, I have a patch which adds a `copy()` method which gets a src
argument, it's currently number 11 of 14 in my patch series.

dispatch.dispatch creates a new uimod.ui object for each request,
hg.remoteui calls ui.copy.
There can legitimately be a bunch of ui objects floating around.
Yuya Nishihara - Feb. 8, 2016, 2:11 p.m.
On Sun, 7 Feb 2016 20:32:16 -0500, timeless wrote:
> Yuya Nishihara wrote:
> > Did you try overriding blackboxui.__init__ ? It should be called with "src"
> > ui when copying ui object. I think global cache is the last option.
> 
> This code already has a global cache of sort for the `lastblackbox` file handle.

Yes. It is clear that there would be no way to avoid global lastblackbox
cache.

> And yes, I have a patch which adds a `copy()` method which gets a src
> argument, it's currently number 11 of 14 in my patch series.
> 
> dispatch.dispatch creates a new uimod.ui object for each request,
> hg.remoteui calls ui.copy.
> There can legitimately be a bunch of ui objects floating around.

So you mean the problem can't be fixed by ui.copy()? Then, global filehandles
dict would be unavoidable.
timeless - Feb. 8, 2016, 6:06 p.m.
Yuya Nishihara wrote:
> So you mean the problem can't be fixed by ui.copy()?

yep

> Then, global filehandles dict would be unavoidable.

Patch

diff --git a/hgext/blackbox.py b/hgext/blackbox.py
--- a/hgext/blackbox.py
+++ b/hgext/blackbox.py
@@ -42,6 +42,22 @@ 
 testedwith = 'internal'
 lastblackbox = None
 
+filehandles = {}
+
+def _openlog(vfs):
+    path = vfs.join('blackbox.log')
+    if path in filehandles:
+        return filehandles[path]
+    filehandles[path] = fp = vfs('blackbox.log', 'a')
+    filehandles[fp] = path
+    return fp
+
+def _closelog(fp):
+    path = filehandles[fp]
+    del filehandles[fp]
+    del filehandles[path]
+    fp.close()
+
 def wrapui(ui):
     class blackboxui(ui.__class__):
         @util.propertycache
@@ -51,33 +67,33 @@ 
         def _openlogfile(self):
             def rotate(oldpath, newpath):
                 try:
-                    os.unlink(newpath)
+                    self._bbvfs.unlink(newpath)
                 except OSError as err:
                     if err.errno != errno.ENOENT:
                         self.debug("warning: cannot remove '%s': %s\n" %
                                    (newpath, err.strerror))
                 try:
                     if newpath:
-                        os.rename(oldpath, newpath)
+                        self._bbvfs.rename(oldpath, newpath)
                 except OSError as err:
                     if err.errno != errno.ENOENT:
                         self.debug("warning: cannot rename '%s' to '%s': %s\n" %
                                    (newpath, oldpath, err.strerror))
 
-            fp = self._bbopener('blackbox.log', 'a')
+            fp = _openlog(self._bbvfs)
             maxsize = self.configbytes('blackbox', 'maxsize', 1048576)
             if maxsize > 0:
-                st = os.fstat(fp.fileno())
+                st = self._bbvfs.fstat(fp)
                 if st.st_size >= maxsize:
                     path = fp.name
-                    fp.close()
+                    _closelog(fp)
                     maxfiles = self.configint('blackbox', 'maxfiles', 7)
                     for i in xrange(maxfiles - 1, 1, -1):
                         rotate(oldpath='%s.%d' % (path, i - 1),
                                newpath='%s.%d' % (path, i))
                     rotate(oldpath=path,
                            newpath=maxfiles > 0 and path + '.1')
-                    fp = self._bbopener('blackbox.log', 'a')
+                    fp = _openlog(self._bbvfs)
             return fp
 
         def log(self, event, *msg, **opts):
@@ -89,13 +105,13 @@ 
 
             if util.safehasattr(self, '_blackbox'):
                 blackbox = self._blackbox
-            elif util.safehasattr(self, '_bbopener'):
+            elif util.safehasattr(self, '_bbvfs'):
                 try:
                     self._blackbox = self._openlogfile()
                 except (IOError, OSError) as err:
                     self.debug('warning: cannot write to blackbox.log: %s\n' %
                                err.strerror)
-                    del self._bbopener
+                    del self._bbvfs
                     self._blackbox = None
                 blackbox = self._blackbox
             else:
@@ -119,7 +135,7 @@ 
                 lastblackbox = blackbox
 
         def setrepo(self, repo):
-            self._bbopener = repo.vfs
+            self._bbvfs = repo.vfs
 
     ui.__class__ = blackboxui