Patchwork dispatch: split global error handling out so it can be reused

login
register
mail settings
Submitter Jun Wu
Date Aug. 9, 2016, 3:46 p.m.
Message ID <b43232adc0f8f6028592.1470757617@x1c>
Download mbox | patch
Permalink /patch/16229/
State Accepted
Headers show

Comments

Jun Wu - Aug. 9, 2016, 3:46 p.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1470757528 -3600
#      Tue Aug 09 16:45:28 2016 +0100
# Node ID b43232adc0f8f60285921bdb65fd0f55710ed3b2
# Parent  3dbc95f3eb31874ab4633f936acff4609714dc41
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r b43232adc0f8
dispatch: split global error handling out so it can be reused

We may want a similar error handling at worker.py. This patch extracts the
error handling logic to "callcatch" so it can be reused.
Yuya Nishihara - Aug. 10, 2016, 12:01 p.m.
On Tue, 9 Aug 2016 16:46:57 +0100, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1470757528 -3600
> #      Tue Aug 09 16:45:28 2016 +0100
> # Node ID b43232adc0f8f60285921bdb65fd0f55710ed3b2
> # Parent  3dbc95f3eb31874ab4633f936acff4609714dc41
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r b43232adc0f8
> dispatch: split global error handling out so it can be reused
> 
> We may want a similar error handling at worker.py. This patch extracts the
> error handling logic to "callcatch" so it can be reused.
> 
> diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
> --- a/mercurial/dispatch.py
> +++ b/mercurial/dispatch.py
> @@ -150,7 +150,7 @@ def _runcatch(req):
>      except ValueError:
>          pass # happens if called in a thread
>  
> -    try:
> +    def _runcatchfunc():
>          try:
>              debugger = 'pdb'
>              debugtrace = {
> @@ -212,6 +212,16 @@ def _runcatch(req):
>              ui.traceback()
>              raise
>  
> +    return callcatch(ui, _runcatchfunc)
> +
> +def callcatch(ui, func):
> +    """call func() with global exception handling
> +
> +    return func() if no exception happens. otherwise do some error handling
> +    and return an exit code accordingly.
> +    """
> +    try:
> +        return func()

The code and docstring look good, but did you try using this function?
I suspect it would create an import cycle.
Jun Wu - Aug. 10, 2016, 1:42 p.m.
Excerpts from Yuya Nishihara's message of 2016-08-10 21:01:25 +0900:
> The code and docstring look good, but did you try using this function?
> I suspect it would create an import cycle.

I tried it using debugshell before sending and it looked okay. I didn't see
cycles?
Yuya Nishihara - Aug. 10, 2016, 1:55 p.m.
On Wed, 10 Aug 2016 14:42:52 +0100, Jun Wu wrote:
> Excerpts from Yuya Nishihara's message of 2016-08-10 21:01:25 +0900:
> > The code and docstring look good, but did you try using this function?
> > I suspect it would create an import cycle.
> 
> I tried it using debugshell before sending and it looked okay. I didn't see
> cycles?

Suppose the next patch will import dispatch.py from worker.py,
test-check-module-imports.t will report the following error:

+  Import cycle: mercurial.dispatch -> mercurial.hg -> mercurial.merge -> mercurial.worker -> mercurial.dispatch
+  Import cycle: mercurial.commands -> mercurial.merge -> mercurial.worker -> mercurial.dispatch -> mercurial.commands
+  Import cycle: mercurial.bundlerepo -> mercurial.localrepo -> mercurial.merge -> mercurial.worker -> mercurial.dispatch -> mercurial.hg -> mercurial.bundlerepo
Jun Wu - Aug. 10, 2016, 2:04 p.m.
Excerpts from Yuya Nishihara's message of 2016-08-10 22:55:26 +0900:
> On Wed, 10 Aug 2016 14:42:52 +0100, Jun Wu wrote:
> > Excerpts from Yuya Nishihara's message of 2016-08-10 21:01:25 +0900:
> > > The code and docstring look good, but did you try using this function?
> > > I suspect it would create an import cycle.
> > 
> > I tried it using debugshell before sending and it looked okay. I didn't see
> > cycles?
> 
> Suppose the next patch will import dispatch.py from worker.py,
> test-check-module-imports.t will report the following error:
> 
> +  Import cycle: mercurial.dispatch -> mercurial.hg -> mercurial.merge -> mercurial.worker -> mercurial.dispatch
> +  Import cycle: mercurial.commands -> mercurial.merge -> mercurial.worker -> mercurial.dispatch -> mercurial.commands
> +  Import cycle: mercurial.bundlerepo -> mercurial.localrepo -> mercurial.merge -> mercurial.worker -> mercurial.dispatch -> mercurial.hg -> mercurial.bundlerepo

It could be solved by importing in the method instead of globally.
commandserver.py already has: "from . import dispatch  # avoid cycle"
Yuya Nishihara - Aug. 10, 2016, 2:37 p.m.
On Wed, 10 Aug 2016 15:04:06 +0100, Jun Wu wrote:
> Excerpts from Yuya Nishihara's message of 2016-08-10 22:55:26 +0900:
> > On Wed, 10 Aug 2016 14:42:52 +0100, Jun Wu wrote:
> > > Excerpts from Yuya Nishihara's message of 2016-08-10 21:01:25 +0900:
> > > > The code and docstring look good, but did you try using this function?
> > > > I suspect it would create an import cycle.
> > > 
> > > I tried it using debugshell before sending and it looked okay. I didn't see
> > > cycles?
> > 
> > Suppose the next patch will import dispatch.py from worker.py,
> > test-check-module-imports.t will report the following error:
> > 
> > +  Import cycle: mercurial.dispatch -> mercurial.hg -> mercurial.merge -> mercurial.worker -> mercurial.dispatch
> > +  Import cycle: mercurial.commands -> mercurial.merge -> mercurial.worker -> mercurial.dispatch -> mercurial.commands
> > +  Import cycle: mercurial.bundlerepo -> mercurial.localrepo -> mercurial.merge -> mercurial.worker -> mercurial.dispatch -> mercurial.hg -> mercurial.bundlerepo
> 
> It could be solved by importing in the method instead of globally.
> commandserver.py already has: "from . import dispatch  # avoid cycle"

Well, that is considered a bad pattern. commandserver (and subrepo) can't
easily avoid import cycles because they send requests back to their upper
layers. There are inherently cycles.

But, your callcatch() could be moved to utility layer except for a few parts
which are tightly bound to command handling. And worker.py won't need these
parts.
Jun Wu - Aug. 10, 2016, 2:47 p.m.
Excerpts from Yuya Nishihara's message of 2016-08-10 23:37:29 +0900:
> Well, that is considered a bad pattern. commandserver (and subrepo) can't
> easily avoid import cycles because they send requests back to their upper
> layers. There are inherently cycles.
> 
> But, your callcatch() could be moved to utility layer except for a few parts
> which are tightly bound to command handling. And worker.py won't need these
> parts.

Probably scmutil.py? I can move it there after this commit.
Yuya Nishihara - Aug. 11, 2016, 8:32 a.m.
On Wed, 10 Aug 2016 15:47:34 +0100, Jun Wu wrote:
> Excerpts from Yuya Nishihara's message of 2016-08-10 23:37:29 +0900:
> > Well, that is considered a bad pattern. commandserver (and subrepo) can't
> > easily avoid import cycles because they send requests back to their upper
> > layers. There are inherently cycles.
> > 
> > But, your callcatch() could be moved to utility layer except for a few parts
> > which are tightly bound to command handling. And worker.py won't need these
> > parts.
> 
> Probably scmutil.py?

Perhaps.

> I can move it there after this commit.

Okay, queued this, thanks. I've updated test-devel-warnings.t in flight.
Jun Wu - Aug. 11, 2016, 12:49 p.m.
Excerpts from Yuya Nishihara's message of 2016-08-11 17:32:03 +0900:
> Okay, queued this, thanks. I've updated test-devel-warnings.t in flight.

Sorry for the inconvenience. I sometimes only run a small subset of tests
for seemingly-trivial patches. I may want to implement patchbomb hooks and
write "# Tested: <short message about tests run>" in patch header.

Patch

diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
--- a/mercurial/dispatch.py
+++ b/mercurial/dispatch.py
@@ -150,7 +150,7 @@  def _runcatch(req):
     except ValueError:
         pass # happens if called in a thread
 
-    try:
+    def _runcatchfunc():
         try:
             debugger = 'pdb'
             debugtrace = {
@@ -212,6 +212,16 @@  def _runcatch(req):
             ui.traceback()
             raise
 
+    return callcatch(ui, _runcatchfunc)
+
+def callcatch(ui, func):
+    """call func() with global exception handling
+
+    return func() if no exception happens. otherwise do some error handling
+    and return an exit code accordingly.
+    """
+    try:
+        return func()
     # Global exception handling, alphabetically
     # Mercurial-specific first, followed by built-in and library exceptions
     except error.AmbiguousCommand as inst: