Patchwork chgserver: wrap ui without calling its constructor

login
register
mail settings
Submitter Jun Wu
Date March 11, 2016, 10:19 p.m.
Message ID <3bd824c35cd84e5259e6.1457734769@x1c>
Download mbox | patch
Permalink /patch/13816/
State Changes Requested
Headers show

Comments

Jun Wu - March 11, 2016, 10:19 p.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1457734734 0
#      Fri Mar 11 22:18:54 2016 +0000
# Node ID 3bd824c35cd84e5259e65be611a40ad2f8080922
# Parent  e94645d05c6a53f08fe81d5209f17d2209894a6f
chgserver: wrap ui without calling its constructor

Calling __init__ could be dangerous if another extension overrides it has
a bug. For example:

  Traceback (most recent call last):
    ...
    File "$HGREPO/hgext/chgserver.py", line 535, in handle
      self.server.hashstate, self.server.baseaddress)
    File "$HGREPO/hgext/chgserver.py", line 346, in __init__
      _newchgui(ui, channeledsystem(fin, fout, 'S')), repo, fin, fout)
    File "$HGREPO/hgext/chgserver.py", line 263, in _newchgui
      return chgui(srcui)
    File "$HGREPO/hgext/chgserver.py", line 234, in __init__
      super(chgui, self).__init__(src)
    File "$HGREPO/hgext/blackbox.py", line 86, in __init__
      self._bbvfs = src._bbvfs
  AttributeError: 'blackboxui' object has no attribute '_bbvfs'

It's pretty bad since the user will get a chg server accepting connections
without being able to serve them. The server cannot even tell the user the
error message since attachio is not executed at this point.

This patch solves the issue by only replacing the system method without
constructing a new ui object.
Yuya Nishihara - March 12, 2016, 5:04 a.m.
On Fri, 11 Mar 2016 22:19:29 +0000, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1457734734 0
> #      Fri Mar 11 22:18:54 2016 +0000
> # Node ID 3bd824c35cd84e5259e65be611a40ad2f8080922
> # Parent  e94645d05c6a53f08fe81d5209f17d2209894a6f
> chgserver: wrap ui without calling its constructor
> 
> Calling __init__ could be dangerous if another extension overrides it has
> a bug. For example:
> 
>   Traceback (most recent call last):
>     ...
>     File "$HGREPO/hgext/chgserver.py", line 535, in handle
>       self.server.hashstate, self.server.baseaddress)
>     File "$HGREPO/hgext/chgserver.py", line 346, in __init__
>       _newchgui(ui, channeledsystem(fin, fout, 'S')), repo, fin, fout)
>     File "$HGREPO/hgext/chgserver.py", line 263, in _newchgui
>       return chgui(srcui)
>     File "$HGREPO/hgext/chgserver.py", line 234, in __init__
>       super(chgui, self).__init__(src)
>     File "$HGREPO/hgext/blackbox.py", line 86, in __init__
>       self._bbvfs = src._bbvfs
>   AttributeError: 'blackboxui' object has no attribute '_bbvfs'
> 
> It's pretty bad since the user will get a chg server accepting connections
> without being able to serve them. The server cannot even tell the user the
> error message since attachio is not executed at this point.
> 
> This patch solves the issue by only replacing the system method without
> constructing a new ui object.

I really don't like this sort of hacks that just hide bugs. If it is a bug
of blackboxui, it must be fixed in blackbox.

> -def _newchgui(srcui, csystem):
> +def _wrapchgui(srcui, defaultcsystem):
>      class chgui(srcui.__class__):
> -        def __init__(self, src=None):
> -            super(chgui, self).__init__(src)
> -            if src:
> -                self._csystem = getattr(src, '_csystem', csystem)
> -            else:
> -                self._csystem = csystem
> -
>          def system(self, cmd, environ=None, cwd=None, onerr=None,
>                     errprefix=None):
>              # copied from mercurial/util.py:system()
> @@ -246,7 +239,11 @@
>              if environ:
>                  env.update((k, py2shell(v)) for k, v in environ.iteritems())
>              env['HG'] = util.hgexecutable()
> -            rc = self._csystem(cmd, env, cwd)
> +            if util.safehasattr(self, '_csystem'):
> +                csystem = self._csystem
> +            else:
> +                csystem = defaultcsystem

It complicates the scope of csystem variable more. I'd rather remove csystem
from __init__.
Jun Wu - March 12, 2016, 5:32 a.m.
On 03/12/2016 05:04 AM, Yuya Nishihara wrote:
> I really don't like this sort of hacks that just hide bugs. If it is a bug
> of blackboxui, it must be fixed in blackbox.

Why is it a hack? I see the pattern "x.__class__ = anotherclass" being
commonly used in other places (dispatch.py, extensions.py, eol.py, keyword.py,
largefiles/reposetup.py, histedit.py, blackbox.py, color.py and fb extensions).

It's not only just about blackbox. It can be any other extension that has a
broken wrapui logic. The chg server will enter a bad state that never recovers
automatically and the client won't print useful error message. The only way to
get actual error message is to use strace.

I think it's severe enough and should be fixed. An extra benefit of skipping
ui.__init__ is to save some time, good for perf.

> It complicates the scope of csystem variable more. I'd rather remove csystem
> from __init__.

I tried setting srcui._csystem directly but due to the current bad ui object
logic it does not work well if others replace our ui object and forgot to
copy _csystem.

I think this is a separate issue about ui/config object design. I have some
ideas on it. If I have free time I plan to get rid of all ui.copy()s and make
config read from files immutabler.
Yuya Nishihara - March 12, 2016, 2:02 p.m.
On Sat, 12 Mar 2016 05:32:19 +0000, Jun Wu wrote:
> On 03/12/2016 05:04 AM, Yuya Nishihara wrote:
> > I really don't like this sort of hacks that just hide bugs. If it is a bug
> > of blackboxui, it must be fixed in blackbox.
> 
> Why is it a hack? I see the pattern "x.__class__ = anotherclass" being
> commonly used in other places (dispatch.py, extensions.py, eol.py, keyword.py,
> largefiles/reposetup.py, histedit.py, blackbox.py, color.py and fb extensions).

Yep, that is a common class wrapping pattern. I didn't mean it is a hack.
I said hiding bugs is a hack. It bypasses chgui.__init__(), but for what?
"because blackboxui.__init__() has bug" isn't appropriate reason.

> It's not only just about blackbox. It can be any other extension that has a
> broken wrapui logic. The chg server will enter a bad state that never recovers
> automatically and the client won't print useful error message. The only way to
> get actual error message is to use strace.

I've sent patches to address this issue.

[snip]

> I think this is a separate issue about ui/config object design. I have some
> ideas on it. If I have free time I plan to get rid of all ui.copy()s and make
> config read from files immutabler.

That will be hard, but I agree the ui will need overhaul.
Jun Wu - March 12, 2016, 7:37 p.m.
On 03/12/2016 02:02 PM, Yuya Nishihara wrote:
> Yep, that is a common class wrapping pattern. I didn't mean it is a hack. I
> said hiding bugs is a hack. It bypasses chgui.__init__(), but for what?
> "because blackboxui.__init__() has bug" isn't appropriate reason.

I will argue my purpose is not to hide bugs in blackbox. Let's forget about
blackbox for now.

The question here is "should we construct the ui object in chgserver?" and
my answer is "unnecessary", since we only want to replace ui.system and it
feels nature to be done using the wrapping pattern without constructing
a new ui object.

I will change the commit message to avoid mentioning blackbox. It should make
the patch look better.

My impression about the issue is like you don't want to use printf in a signal
handler in C since it's unsafe. chgserver before attachio is somehow similar
and you just want to avoid calling other unsafe stuff.

> I've sent patches to address this issue.

Thanks! They also make sense. I think we can apply both yours and this patch.

> That will be hard, but I agree the ui will need overhaul.

Yep. That's why I want to avoid constructing new ui objects. There are too
many ui objects floating around the code base. By having a lot of them, the
code is likely to have outdated objects that helps to hide bugs.
Yuya Nishihara - March 13, 2016, 4:21 a.m.
On Sat, 12 Mar 2016 19:37:54 +0000, Jun Wu wrote:
> On 03/12/2016 02:02 PM, Yuya Nishihara wrote:
> > Yep, that is a common class wrapping pattern. I didn't mean it is a hack. I
> > said hiding bugs is a hack. It bypasses chgui.__init__(), but for what?
> > "because blackboxui.__init__() has bug" isn't appropriate reason.
> 
> I will argue my purpose is not to hide bugs in blackbox. Let's forget about
> blackbox for now.
> 
> The question here is "should we construct the ui object in chgserver?" and
> my answer is "unnecessary", since we only want to replace ui.system and it
> feels nature to be done using the wrapping pattern without constructing
> a new ui object.

Well, I designed it to create a new ui so that chgcmdserver can be instantiated
without forking. If the server isn't fork-per-request, the "ui" object would
live longer than the "chgcmdserver" instance. That's why chgcmdserver should
create a copy of "ui" before modifying it.
Jun Wu - March 13, 2016, 11:11 a.m.
On 03/13/2016 04:21 AM, Yuya Nishihara wrote:
> Well, I designed it to create a new ui so that chgcmdserver can be
> instantiated without forking. If the server isn't fork-per-request

But fork won't go away soon right? I think it cannot happen before some ui
object refactoring, which is not happening soon either.

There are 45 "self.ui = ui" and it's causing trouble because they may keep
outdated ui objects. By trouble I mean they result in hard-to-notice and
hard-to-debug bugs. Some are exposed in tests. But I think most of them
are invisible and will just surprise people at some time.

For example, I had spent a lot of time on test-progress.t, ended up with
printing ids and stacktraces with every ui creation. It turns out the
"ui.copy()" in runcommand of commandsesrver.py causes inconsistency with the
old ui object kept in progress.py by "self.ui = ui".

That's why I want to get rid of "ui.{copy,__init__}" very strongly.

The possible bug hidden by this chgui is not easily exposed because extensions 
do not use "system" often for interactive commands. But I do believe it will
hide bugs because of the 45 "self.ui = ui"s.

So I still want this in for now. It's much easier than fixing "self.ui = ui"s 
one by one or do the huge ui refactoring. It's also easy to be reverted once we
are ready to get rid of fork. I will be happy to add detailed comments about
the story if we decide to accept the patch.
Yuya Nishihara - March 13, 2016, 12:12 p.m.
On Sun, 13 Mar 2016 11:11:07 +0000, Jun Wu wrote:
> On 03/13/2016 04:21 AM, Yuya Nishihara wrote:
> > Well, I designed it to create a new ui so that chgcmdserver can be
> > instantiated without forking. If the server isn't fork-per-request
> 
> But fork won't go away soon right? I think it cannot happen before some ui
> object refactoring, which is not happening soon either.

I don't think ui is the blocker. Instead, we'll have to eliminate globals that
are modified while running command, which is also hard and won't happen soon.

> There are 45 "self.ui = ui" and it's causing trouble because they may keep
> outdated ui objects. By trouble I mean they result in hard-to-notice and
> hard-to-debug bugs. Some are exposed in tests. But I think most of them
> are invisible and will just surprise people at some time.
> 
> For example, I had spent a lot of time on test-progress.t, ended up with
> printing ids and stacktraces with every ui creation. It turns out the
> "ui.copy()" in runcommand of commandsesrver.py causes inconsistency with the
> old ui object kept in progress.py by "self.ui = ui".
> 
> That's why I want to get rid of "ui.{copy,__init__}" very strongly.

Hmm, but you can't eliminate all of these copies, right? What's the benefit to
skip only one copy at chgcmdserver.__init__? It will soon be copied at
runcommand() anyway.

> The possible bug hidden by this chgui is not easily exposed because extensions 
> do not use "system" often for interactive commands. But I do believe it will
> hide bugs because of the 45 "self.ui = ui"s.

In order to expose such bugs, I'd rather make chgui.system() do crash if it
isn't initialized appropriately.

> It's also easy to be reverted once we
> are ready to get rid of fork. I will be happy to add detailed comments about
> the story if we decide to accept the patch.

Well, if you want to change _newchgui() to _wrapchgui(), move _wrapchgui() to
uisetup(). That will make sure the class wrapping is processed only once.
Jun Wu - March 13, 2016, 3:10 p.m.
On 03/13/2016 12:12 PM, Yuya Nishihara wrote:
> Hmm, but you can't eliminate all of these copies, right? What's the benefit
> to skip only one copy at chgcmdserver.__init__? It will soon be copied at
> runcommand() anyway.

Another patch will stop runcommand() from copying ui. It will make
test-progress.t result better.

> Well, if you want to change _newchgui() to _wrapchgui(), move _wrapchgui()
> to uisetup(). That will make sure the class wrapping is processed only
> once.

I have considered this way in the first place. I didn't do it because
_csystem is not ready at that time.

Let's do it + start killing ui.copy (which I just found have filesystem race
condition issues).
Yuya Nishihara - March 13, 2016, 3:52 p.m.
On Sun, 13 Mar 2016 15:10:08 +0000, Jun Wu wrote:
> On 03/13/2016 12:12 PM, Yuya Nishihara wrote:
> > Hmm, but you can't eliminate all of these copies, right? What's the benefit
> > to skip only one copy at chgcmdserver.__init__? It will soon be copied at
> > runcommand() anyway.
> 
> Another patch will stop runcommand() from copying ui. It will make
> test-progress.t result better.

Can you send it first? It smells like a controversial patch. ;)

> > Well, if you want to change _newchgui() to _wrapchgui(), move _wrapchgui()
> > to uisetup(). That will make sure the class wrapping is processed only
> > once.
> 
> I have considered this way in the first place. I didn't do it because
> _csystem is not ready at that time.
> 
> Let's do it + start killing ui.copy (which I just found have filesystem race
> condition issues).

Or should I send patches that will remove chgui completely? I didn't write
them yet, but it should be possible. Basic idea is to move ui-layer code from
util.system() to ui.system(), and make ui._csystem swappable.
Jun Wu - March 13, 2016, 7:33 p.m.
On 03/13/2016 03:52 PM, Yuya Nishihara wrote:
>> Another patch will stop runcommand() from copying ui. It will make
>> test-progress.t result better.
>
> Can you send it first? It smells like a controversial patch. ;)

I guess the argument will lean toward an issue with progress.py.
I will have a quick look at progbar logic when I come to test-progress.t
again.

A side note is _renewui is not ideal. Ideally it only touches configs
of an existing global ui and local ui object without creating new uis.
But that's currently not possible since we have no access to the global
ui object.

It seems I may want to start the ui/config refactoring sooner than I expect.

>> Let's do it + start killing ui.copy (which I just found have filesystem
>> race condition issues).

Sorry, the filesystem race condition is not true.

> Or should I send patches that will remove chgui completely? I didn't write
> them yet, but it should be possible. Basic idea is to move ui-layer code
> from util.system() to ui.system(), and make ui._csystem swappable.

Do you mean adding a parameter "csystem" (name not decided) to util.system and
pass "self._system" in "ui.system" ? It can solve the __init__ exception issue
and get rid of duplicated code, which I think is reasonable. I can write this
patch if you are busy with other stuff.
Yuya Nishihara - March 14, 2016, 2:02 p.m.
On Sun, 13 Mar 2016 19:33:21 +0000, Jun Wu wrote:
> On 03/13/2016 03:52 PM, Yuya Nishihara wrote:
> > Or should I send patches that will remove chgui completely? I didn't write
> > them yet, but it should be possible. Basic idea is to move ui-layer code
> > from util.system() to ui.system(), and make ui._csystem swappable.
> 
> Do you mean adding a parameter "csystem" (name not decided) to util.system and
> pass "self._system" in "ui.system" ? It can solve the __init__ exception issue
> and get rid of duplicated code, which I think is reasonable. I can write this
> patch if you are busy with other stuff.

My idea is somewhat different:

  ui._system = util.system  # default
  ui._system = channeledsystem(fin, fout, 'S')  # chg

I haven't started writing the patches yet for two reasons:

 a) I considered it would be a pure clean-up series.
 b) I might want to extend the 'S' channel to handle pager request. If the
    pager extension gets into the core, it might have more precise logic when
    to start a pager, such as ui.startpager(). Current "getpager" command can't
    handle such situation.
Jun Wu - March 14, 2016, 2:08 p.m.
On 03/14/2016 02:02 PM, Yuya Nishihara wrote:
> My idea is somewhat different:
>
>    ui._system = util.system  # default
>    ui._system = channeledsystem(fin, fout, 'S')  # chg
>
> I haven't started writing the patches yet for two reasons:
>
>   a) I considered it would be a pure clean-up series.
>   b) I might want to extend the 'S' channel to handle pager request. If the
>      pager extension gets into the core, it might have more precise logic when
>      to start a pager, such as ui.startpager(). Current "getpager" command can't
>      handle such situation.

Looks better. The patch is harder than I expected because we still have
the csystem scope issue. _renewui currently needs to create new ui objects
and it cannot access the intended default csystem. It seems I have to start the
refactoring before making _renewui better.
Jun Wu - March 14, 2016, 2:09 p.m.
On 03/14/2016 02:02 PM, Yuya Nishihara wrote:
> My idea is somewhat different:
>
>    ui._system = util.system  # default
>    ui._system = channeledsystem(fin, fout, 'S')  # chg
>
> I haven't started writing the patches yet for two reasons:
>
>   a) I considered it would be a pure clean-up series.
>   b) I might want to extend the 'S' channel to handle pager request. If the
>      pager extension gets into the core, it might have more precise logic when
>      to start a pager, such as ui.startpager(). Current "getpager" command can't
>      handle such situation.

Looks better. The patch is harder than I expected because we still have
the csystem scope issue. _renewui currently needs to create new ui objects
and it cannot access the intended default csystem. It seems I have to start the
ui/config (at least config) refactoring before making _renewui better.
Augie Fackler - March 15, 2016, 2:39 a.m.
> On Mar 14, 2016, at 10:02 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> 
> I haven't started writing the patches yet for two reasons:
> 
> a) I considered it would be a pure clean-up series.
> b) I might want to extend the 'S' channel to handle pager request. If the
>    pager extension gets into the core, it might have more precise logic when
>    to start a pager, such as ui.startpager(). Current "getpager" command can't
>    handle such situation.

I’ve been slowly tinkering with cleaning up the pager code, and I’ve also come to the conclusion that something along the lines of ui.pager() is going to be required. So far, I’ve made it this far in my analysis:

 * ui.pager() probably needs to take the “command type” or similar - that is, it’d be something like ‘ui.pager(“diff")’  or ‘ui.pager(“log”)’, so that it’s easy(-ish) to disable entire categories of pagers trivially.

 * Moving to ui.pager() probably means we can ditch the entire notion of the “attend” list for anything other than people wanting to forcibly page commands that don’t make sense to be paged (like summary, maybe?)

 * There probably needs to be a registry of post-pager-activation actions. At a minimum, the color extension needs to (potentially) reconfigure how it’s emitting color labels once the pager activates.

Some probable wins out of this:

 * User-defined aliases can still be paged (or not) by default in a sane way

 * commands which want to emit a progress bar until some results are ready (grep?) can emit progress info until the first line of output becomes available, then enable the pager (which would disable progress bars).

Anyway, I figured I should share those notes since you’re also thinking about this. Maybe we should talk at the sprint.

Patch

diff --git a/hgext/chgserver.py b/hgext/chgserver.py
--- a/hgext/chgserver.py
+++ b/hgext/chgserver.py
@@ -223,15 +223,8 @@ 
             _envvarre.search(cmddef.definition)):
             del cmdtable[name]
 
-def _newchgui(srcui, csystem):
+def _wrapchgui(srcui, defaultcsystem):
     class chgui(srcui.__class__):
-        def __init__(self, src=None):
-            super(chgui, self).__init__(src)
-            if src:
-                self._csystem = getattr(src, '_csystem', csystem)
-            else:
-                self._csystem = csystem
-
         def system(self, cmd, environ=None, cwd=None, onerr=None,
                    errprefix=None):
             # copied from mercurial/util.py:system()
@@ -246,7 +239,11 @@ 
             if environ:
                 env.update((k, py2shell(v)) for k, v in environ.iteritems())
             env['HG'] = util.hgexecutable()
-            rc = self._csystem(cmd, env, cwd)
+            if util.safehasattr(self, '_csystem'):
+                csystem = self._csystem
+            else:
+                csystem = defaultcsystem
+            rc = csystem(cmd, env, cwd)
             if rc and onerr:
                 errmsg = '%s %s' % (os.path.basename(cmd.split(None, 1)[0]),
                                     util.explainexit(rc)[0])
@@ -255,7 +252,7 @@ 
                 raise onerr(errmsg)
             return rc
 
-    return chgui(srcui)
+    srcui.__class__ = chgui
 
 def _renewui(srcui, args=None):
     if not args:
@@ -337,8 +334,8 @@ 
 
 class chgcmdserver(commandserver.server):
     def __init__(self, ui, repo, fin, fout, sock, hashstate, baseaddress):
-        super(chgcmdserver, self).__init__(
-            _newchgui(ui, channeledsystem(fin, fout, 'S')), repo, fin, fout)
+        _wrapchgui(ui, channeledsystem(fin, fout, 'S'))
+        super(chgcmdserver, self).__init__(ui, repo, fin, fout)
         self.clientsock = sock
         sock.setblocking(1)
         self._oldios = []  # original (self.ch, ui.fp, fd) before "attachio"