changeset 34124:cf04db16f583

blackbox: do not cache file objects

Having the blackbox file objects cached in `ui._bbfp` could in theory be
troublesome if multiple processes (ex. chg servers) have file objects
referring to a same file. (Although I spent some time and failed to build a
convincing test case)

This patch makes blackbox re-open the file every time to make the situation
better. Ideally we also need proper locking.

The caching logic traces back to the commit introducing blackbox
(18242716a). That commit does not have details about why caching is
necessary.  Consider the fact that blackbox logs are not many, it seems fine
to remove the fp cache to be more confident.

Differential Revision: https://phab.mercurial-scm.org/D650

--- a/hgext/blackbox.py	Wed Sep 06 20:54:53 2017 -0700
+++ b/hgext/blackbox.py	Wed Sep 06 21:08:59 2017 -0700
@@ -80,7 +80,6 @@
             if src is None:
                 self._partialinit()
             else:
-                self._bbfp = getattr(src, '_bbfp', None)
                 self._bbinlog = False
                 self._bbrepo = getattr(src, '_bbrepo', None)
                 self._bbvfs = getattr(src, '_bbvfs', None)
@@ -88,7 +87,6 @@
         def _partialinit(self):
             if util.safehasattr(self, '_bbvfs'):
                 return
-            self._bbfp = None
             self._bbinlog = False
             self._bbrepo = None
             self._bbvfs = None
@@ -143,16 +141,7 @@
             if not '*' in self.track and not event in self.track:
                 return
 
-            if self._bbfp:
-                ui = self
-            elif self._bbvfs:
-                try:
-                    self._bbfp = self._openlogfile()
-                except (IOError, OSError) as err:
-                    self.debug('warning: cannot write to blackbox.log: %s\n' %
-                               err.strerror)
-                    del self._bbvfs
-                    self._bbfp = None
+            if self._bbvfs:
                 ui = self
             else:
                 # certain ui instances exist outside the context of
@@ -160,12 +149,12 @@
                 # was seen.
                 ui = lastui
 
-            if not ui or not ui._bbfp:
+            if not ui:
                 return
             if not lastui or ui._bbrepo:
                 lastui = ui
             if ui._bbinlog:
-                # recursion guard
+                # recursion and failure guard
                 return
             try:
                 ui._bbinlog = True
@@ -188,19 +177,21 @@
                 else:
                     src = ''
                 try:
-                    fp = ui._bbfp
                     fmt = '%s %s @%s%s (%s)%s> %s'
                     args = (date, user, rev, changed, pid, src, formattedmsg)
-                    fp.write(fmt % args)
-                    fp.flush()
-                except IOError as err:
+                    with ui._openlogfile() as fp:
+                        fp.write(fmt % args)
+                except (IOError, OSError) as err:
                     self.debug('warning: cannot write to blackbox.log: %s\n' %
                                err.strerror)
+                    # do not restore _bbinlog intentionally to avoid failed
+                    # logging again
+                else:
+                    ui._bbinlog = False
             finally:
-                ui._bbinlog = False
+                pass
 
         def setrepo(self, repo):
-            self._bbfp = None
             self._bbinlog = False
             self._bbrepo = repo
             self._bbvfs = repo.vfs