Patchwork [10,of,10] extensions: devel-warn ui object escaping from ui/extsetup

login
register
mail settings
Submitter Jun Wu
Date June 30, 2016, 4:59 p.m.
Message ID <caf11c2daf24c7f94a50.1467305945@x1c>
Download mbox | patch
Permalink /patch/15688/
State Changes Requested
Delegated to: Yuya Nishihara
Headers show

Comments

Jun Wu - June 30, 2016, 4:59 p.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1467296787 -3600
#      Thu Jun 30 15:26:27 2016 +0100
# Node ID caf11c2daf24c7f94a506acb440e80ea37898870
# Parent  7c2dc6643d1c37dbc388ce4ac7112341f0926f28
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r caf11c2daf24
extensions: devel-warn ui object escaping from ui/extsetup

Storing a reference to the ui object in uisetup or extsetup can cause troubles
since the ui object can be outdated because of ui.copy()s or ui.__init__()s.
It's especially an issue with chgserver since it's designed to call uisetup
only once.

This patch adds a develwarn to notify developers this usage is discouraged.

See also https://twitter.com/durin42/status/739836476318318592 .
Yuya Nishihara - July 3, 2016, 4:53 a.m.
On Thu, 30 Jun 2016 17:59:05 +0100, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1467296787 -3600
> #      Thu Jun 30 15:26:27 2016 +0100
> # Node ID caf11c2daf24c7f94a506acb440e80ea37898870
> # Parent  7c2dc6643d1c37dbc388ce4ac7112341f0926f28
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r caf11c2daf24
> extensions: devel-warn ui object escaping from ui/extsetup

> --- a/mercurial/extensions.py
> +++ b/mercurial/extensions.py
> @@ -7,8 +7,10 @@
>  
>  from __future__ import absolute_import
>  
> +import contextlib
>  import imp
>  import os
> +import sys
>  
>  from .i18n import (
>      _,
> @@ -127,20 +129,31 @@ def load(ui, name, path):
>          fn(loaded=True)
>      return mod
>  
> +@contextlib.contextmanager
> +def _checkuiescape(name, ui):
> +    nref = sys.getrefcount(ui)
> +    yield
> +    nrefchange = sys.getrefcount(ui) - nref
> +    if nrefchange > 0 and (ui.configbool('devel', 'all-warnings')
> +                           or ui.configbool('devel', 'check-uiescape')):
> +        ui.develwarn('"ui" escaped from %s' % name, stacklevel=None)

I'm sure this won't work on PyPy.

Can you make a separate series of the refcount patches? Reviewing them will
require expertise about CPython.

Patch

diff --git a/mercurial/extensions.py b/mercurial/extensions.py
--- a/mercurial/extensions.py
+++ b/mercurial/extensions.py
@@ -7,8 +7,10 @@ 
 
 from __future__ import absolute_import
 
+import contextlib
 import imp
 import os
+import sys
 
 from .i18n import (
     _,
@@ -127,20 +129,31 @@  def load(ui, name, path):
         fn(loaded=True)
     return mod
 
+@contextlib.contextmanager
+def _checkuiescape(name, ui):
+    nref = sys.getrefcount(ui)
+    yield
+    nrefchange = sys.getrefcount(ui) - nref
+    if nrefchange > 0 and (ui.configbool('devel', 'all-warnings')
+                           or ui.configbool('devel', 'check-uiescape')):
+        ui.develwarn('"ui" escaped from %s' % name, stacklevel=None)
+
 def _runuisetup(name, ui):
-    uisetup = getattr(_extensions[name], 'uisetup', None)
-    if uisetup:
-        uisetup(ui)
+    with _checkuiescape('%s.uisetup' % name, ui):
+        uisetup = getattr(_extensions[name], 'uisetup', None)
+        if uisetup:
+            uisetup(ui)
 
 def _runextsetup(name, ui):
-    extsetup = getattr(_extensions[name], 'extsetup', None)
-    if extsetup:
-        try:
-            extsetup(ui)
-        except TypeError:
-            if extsetup.func_code.co_argcount != 0:
-                raise
-            extsetup() # old extsetup with no ui argument
+    with _checkuiescape('%s.extsetup' % name, ui):
+        extsetup = getattr(_extensions[name], 'extsetup', None)
+        if extsetup:
+            try:
+                extsetup(ui)
+            except TypeError:
+                if extsetup.func_code.co_argcount != 0:
+                    raise
+                extsetup() # old extsetup with no ui argument
 
 def loadall(ui):
     result = ui.configitems("extensions")
diff --git a/mercurial/ui.py b/mercurial/ui.py
--- a/mercurial/ui.py
+++ b/mercurial/ui.py
@@ -1164,6 +1164,7 @@  class ui(object):
                 return
         msg = 'devel-warn: ' + msg
         if stacklevel is None:
+            self.write_err('%s\n' % msg)
             self.log('develwarn', msg)
             return
         stacklevel += 1 # get in develwarn
diff --git a/tests/test-devel-warnings.t b/tests/test-devel-warnings.t
--- a/tests/test-devel-warnings.t
+++ b/tests/test-devel-warnings.t
@@ -1,4 +1,3 @@ 
-
   $ cat << EOF > buggylocking.py
   > """A small extension that tests our developer warnings
   > """
@@ -74,6 +73,18 @@ 
   > all-warnings=1
   > EOF
 
+  $ cat << EOF > uiescape.py
+  > a = []
+  > def uisetup(ui):
+  >     ui.write('ui escaping\n')
+  >     a.append(ui)
+  > EOF
+
+  $ hg --config extensions.uiescape=uiescape.py version -q
+  ui escaping
+  devel-warn: "ui" escaped from uiescape.uisetup
+  Mercurial * (glob)
+
   $ hg init lock-checker
   $ cd lock-checker
   $ hg buggylocking