| 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
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.
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.
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.
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