Patchwork chgserver: do not print traceback on SystemExit

login
register
mail settings
Submitter Jun Wu
Date April 10, 2016, 12:42 a.m.
Message ID <3efb9e44023397f21243.1460248979@x1c>
Download mbox | patch
Permalink /patch/14478/
State Changes Requested
Delegated to: Yuya Nishihara
Headers show

Comments

Jun Wu - April 10, 2016, 12:42 a.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1460248929 -3600
#      Sun Apr 10 01:42:09 2016 +0100
# Node ID 3efb9e44023397f212437d89bfd7268cb5a15df6
# Parent  a212504cf958742becab6fe518dab9b838f5bbf4
chgserver: do not print traceback on SystemExit

Before the patch, if some extension uses "sys.exit", chg will print an extra
backtrace. We have ignored KeyboardInterrupt already, and it makes It makes
sense to ignore SystemExit as well.

This patch addresses the issue by adding SystemExit to the ignoring list.
timeless - April 10, 2016, 2:14 a.m.
Jun Wu wrote:
> backtrace. We have ignored KeyboardInterrupt already, and it makes It makes
> sense to ignore SystemExit as well.

Unfortunately this sentence doesn't make sense :(

probably drop "makes It makes"
Jun Wu - April 10, 2016, 2:17 a.m.
On 04/10/2016 03:14 AM, timeless wrote:
> Unfortunately this sentence doesn't make sense :(
> probably drop "makes It makes"

Sorry about these two patches. :(

There must be something wrong with my editor.
Yuya Nishihara - April 10, 2016, 11:36 a.m.
On Sun, 10 Apr 2016 01:42:59 +0100, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1460248929 -3600
> #      Sun Apr 10 01:42:09 2016 +0100
> # Node ID 3efb9e44023397f212437d89bfd7268cb5a15df6
> # Parent  a212504cf958742becab6fe518dab9b838f5bbf4
> chgserver: do not print traceback on SystemExit
> 
> Before the patch, if some extension uses "sys.exit", chg will print an extra
> backtrace. We have ignored KeyboardInterrupt already, and it makes It makes
> sense to ignore SystemExit as well.

Can you show the stacktrace? I'm interested in which command can execute
that ugly sys.exit(). For "runcommand", SystemExit should be caught at dispatch.

> diff --git a/hgext/chgserver.py b/hgext/chgserver.py
> --- a/hgext/chgserver.py
> +++ b/hgext/chgserver.py
> @@ -559,7 +559,7 @@
>              except IOError as inst:
>                  if inst.errno != errno.EPIPE:
>                      raise
> -            except KeyboardInterrupt:
> +            except (KeyboardInterrupt, SystemExit):
>                  pass

If it is a common problem, this line should be copied to commandserver.py.
Jun Wu - April 10, 2016, 12:29 p.m.
On 04/10/2016 12:36 PM, Yuya Nishihara wrote:
> Can you show the stacktrace? I'm interested in which command can execute
> that ugly sys.exit(). For "runcommand", SystemExit should be caught at
> dispatch.

This was a little hack at facebook that suppresses the error message otherwise
would be printed by dispatch.py:

https://bitbucket.org/facebook/hg-experimental/src/a5aaa/errorredirect.py#errorredirect.py-48

> If it is a common problem, this line should be copied to commandserver.py.

Good point. I will do that in V2.
Yuya Nishihara - April 10, 2016, 1:54 p.m.
On Sun, 10 Apr 2016 13:29:58 +0100, Jun Wu wrote:
> On 04/10/2016 12:36 PM, Yuya Nishihara wrote:
> > Can you show the stacktrace? I'm interested in which command can execute
> > that ugly sys.exit(). For "runcommand", SystemExit should be caught at
> > dispatch.  
> 
> This was a little hack at facebook that suppresses the error message otherwise
> would be printed by dispatch.py:
> 
> https://bitbucket.org/facebook/hg-experimental/src/a5aaa/errorredirect.py#errorredirect.py-48

I guess it will be changed to hook dispatch.handlecommandexception(), which
was recently added. And hopefully we won't need V2.

> > If it is a common problem, this line should be copied to commandserver.py.
> 
> Good point. I will do that in V2.
Jun Wu - April 10, 2016, 2:22 p.m.
On 04/10/2016 02:54 PM, Yuya Nishihara wrote:
> I guess it will be changed to hook dispatch.handlecommandexception(), which
> was recently added. And hopefully we won't need V2.

Yep. That's also the reason why it was added. I will graft related commits
and make things work (I'm doing the release internally).

Patch

diff --git a/hgext/chgserver.py b/hgext/chgserver.py
--- a/hgext/chgserver.py
+++ b/hgext/chgserver.py
@@ -559,7 +559,7 @@ 
             except IOError as inst:
                 if inst.errno != errno.EPIPE:
                     raise
-            except KeyboardInterrupt:
+            except (KeyboardInterrupt, SystemExit):
                 pass
             finally:
                 sv.cleanup()