Patchwork [4,of,4] serve: ignore os.setsid EPERM error

login
register
mail settings
Submitter Jun Wu
Date March 9, 2016, 2:23 a.m.
Message ID <d2f89bd9d8cf5b0451d2.1457490206@x1c>
Download mbox | patch
Permalink /patch/13700/
State Rejected
Delegated to: Yuya Nishihara
Headers show

Comments

Jun Wu - March 9, 2016, 2:23 a.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1457487748 0
#      Wed Mar 09 01:42:28 2016 +0000
# Node ID d2f89bd9d8cf5b0451d282ad0517b689234a1734
# Parent  e18454eee99d96f1dc16833c98951cadc50a3b1c
serve: ignore os.setsid EPERM error

Before this patch, if a user tries to run "hg serve" manually with the flag
"--daemon-postexec", they will probably get an error like: "OSError: [Errno
1] Operation not permitted". This patch ignores the error.
Yuya Nishihara - March 11, 2016, 8:14 a.m.
On Wed, 9 Mar 2016 02:23:26 +0000, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1457487748 0
> #      Wed Mar 09 01:42:28 2016 +0000
> # Node ID d2f89bd9d8cf5b0451d282ad0517b689234a1734
> # Parent  e18454eee99d96f1dc16833c98951cadc50a3b1c
> serve: ignore os.setsid EPERM error
> 
> Before this patch, if a user tries to run "hg serve" manually with the flag
> "--daemon-postexec", they will probably get an error like: "OSError: [Errno
> 1] Operation not permitted". This patch ignores the error.

Does it happen in real use case? It would be harmless to ignore EPERM, but
seems not correct as --daemon-postexec should be called after fork().

> --- a/mercurial/cmdutil.py
> +++ b/mercurial/cmdutil.py
> @@ -829,7 +829,7 @@
>      if opts['daemon_postexec']:
>          try:
>              os.setsid()
> -        except AttributeError:
> +        except OSError:

AttributeError is raised on Windows.
Jun Wu - March 11, 2016, 10:54 a.m.
On 03/11/2016 08:14 AM, Yuya Nishihara wrote:
> Does it happen in real use case? It would be harmless to ignore EPERM, but
> seems not correct as --daemon-postexec should be called after fork().

I sometimes set ipdb and manually start the server. I think power users
may want to control their servers manually in some cases.

> AttributeError is raised on Windows.

Thanks for pointing this out. I probably should setup Windows test environment
for these change.
Yuya Nishihara - March 11, 2016, 11:35 a.m.
On Fri, 11 Mar 2016 10:54:42 +0000, Jun Wu wrote:
> On 03/11/2016 08:14 AM, Yuya Nishihara wrote:
> > Does it happen in real use case? It would be harmless to ignore EPERM, but
> > seems not correct as --daemon-postexec should be called after fork().
> 
> I sometimes set ipdb and manually start the server. I think power users
> may want to control their servers manually in some cases.

Okay, then, can you change it to a warning so that we don't miss a bad use
of --daemon-postexec?
Jun Wu - March 11, 2016, 12:48 p.m.
On 03/11/2016 11:35 AM, Yuya Nishihara wrote:
> Okay, then, can you change it to a warning so that we don't miss a bad use
> of --daemon-postexec?

ui is not available in the function. I don't want to pollute stderr.
I will drop this patch for now. It is not that important.

Patch

diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -829,7 +829,7 @@ 
     if opts['daemon_postexec']:
         try:
             os.setsid()
-        except AttributeError:
+        except OSError:
             pass
         for inst in opts['daemon_postexec']:
             if inst.startswith('unlink:'):