Patchwork [2,of,3] killdaemons: use posixfile to avoid intermittent unlink errors on Windows

login
register
mail settings
Submitter Matt Harbison
Date May 15, 2017, 4:08 a.m.
Message ID <809709930080937b6b56.1494821311@Envy>
Download mbox | patch
Permalink /patch/20625/
State Changes Requested
Delegated to: Yuya Nishihara
Headers show

Comments

Matt Harbison - May 15, 2017, 4:08 a.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1494808413 14400
#      Sun May 14 20:33:33 2017 -0400
# Node ID 809709930080937b6b56e5cad285798f29a10280
# Parent  024271e90987e5794dab6eb00844467065fae7e4
killdaemons: use posixfile to avoid intermittent unlink errors on Windows

This is the aforementioned fix for the occasional cleanup error with #serve
enabled.  There are a handful of tests that neglect to kill the daemons they
spawned, and this code is doing a last ditch reap of them.  The test that got
flagged was non-deterministic, and I've seen up to 3 fail in the same run.

The problem with trying to import the mercurial module is that while it is
available for running the individual *.t files, it is not in sys.path for
run-tests.py itself.  I couldn't think of any other way to make this work, and
not affect sys.path for the indiviual tests.  (The main source tree _is_ in
PYTHONPATH when this is imported from run-tests.py.)
Yuya Nishihara - May 15, 2017, 2:19 p.m.
On Mon, 15 May 2017 00:08:31 -0400, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1494808413 14400
> #      Sun May 14 20:33:33 2017 -0400
> # Node ID 809709930080937b6b56e5cad285798f29a10280
> # Parent  024271e90987e5794dab6eb00844467065fae7e4
> killdaemons: use posixfile to avoid intermittent unlink errors on Windows
> 
> This is the aforementioned fix for the occasional cleanup error with #serve
> enabled.  There are a handful of tests that neglect to kill the daemons they
> spawned, and this code is doing a last ditch reap of them.  The test that got
> flagged was non-deterministic, and I've seen up to 3 fail in the same run.
> 
> The problem with trying to import the mercurial module is that while it is
> available for running the individual *.t files, it is not in sys.path for
> run-tests.py itself.  I couldn't think of any other way to make this work, and
> not affect sys.path for the indiviual tests.  (The main source tree _is_ in
> PYTHONPATH when this is imported from run-tests.py.)
> 
> diff --git a/tests/killdaemons.py b/tests/killdaemons.py
> --- a/tests/killdaemons.py
> +++ b/tests/killdaemons.py
> @@ -7,6 +7,16 @@
>  import sys
>  import time
>  
> +# PYTHONPATH contains the hg source tree when invoked from ./run-tests, but
> +# sys.path does not, and importing mercurial fails.  The first import works from
> +# the .t files without editing the path.
> +try:
> +    from mercurial.util import posixfile
> +except ImportError:
> +    srctree = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
> +    sys.path.insert(1, srctree)
> +    from mercurial.util import posixfile

sys.path is global. I slightly prefer moving this hack to run-tests.py if
it's okay for run-tests.py to depend on Mercurial modules.
Matt Harbison - May 16, 2017, 1:40 a.m.
On Mon, 15 May 2017 10:19:19 -0400, Yuya Nishihara <yuya@tcha.org> wrote:

> On Mon, 15 May 2017 00:08:31 -0400, Matt Harbison wrote:
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison@yahoo.com>
>> # Date 1494808413 14400
>> #      Sun May 14 20:33:33 2017 -0400
>> # Node ID 809709930080937b6b56e5cad285798f29a10280
>> # Parent  024271e90987e5794dab6eb00844467065fae7e4
>> killdaemons: use posixfile to avoid intermittent unlink errors on  
>> Windows
>>
>> This is the aforementioned fix for the occasional cleanup error with  
>> #serve
>> enabled.  There are a handful of tests that neglect to kill the daemons  
>> they
>> spawned, and this code is doing a last ditch reap of them.  The test  
>> that got
>> flagged was non-deterministic, and I've seen up to 3 fail in the same  
>> run.
>>
>> The problem with trying to import the mercurial module is that while it  
>> is
>> available for running the individual *.t files, it is not in sys.path  
>> for
>> run-tests.py itself.  I couldn't think of any other way to make this  
>> work, and
>> not affect sys.path for the indiviual tests.  (The main source tree  
>> _is_ in
>> PYTHONPATH when this is imported from run-tests.py.)
>>
>> diff --git a/tests/killdaemons.py b/tests/killdaemons.py
>> --- a/tests/killdaemons.py
>> +++ b/tests/killdaemons.py
>> @@ -7,6 +7,16 @@
>>  import sys
>>  import time
>>
>> +# PYTHONPATH contains the hg source tree when invoked from  
>> ./run-tests, but
>> +# sys.path does not, and importing mercurial fails.  The first import  
>> works from
>> +# the .t files without editing the path.
>> +try:
>> +    from mercurial.util import posixfile
>> +except ImportError:
>> +    srctree =  
>> os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
>> +    sys.path.insert(1, srctree)
>> +    from mercurial.util import posixfile
>
> sys.path is global. I slightly prefer moving this hack to run-tests.py if
> it's okay for run-tests.py to depend on Mercurial modules.

I'm not sure if the dependency is OK, but I wasn't sure what else to do.   
(util.posixfile is a wrapper around osutil.posixfile, so it's not like it  
can be copy/pasted.)  When I was working on the egg problem last weekend,  
I was sure that $TESTTMP was in $PYTHONPATH, but I don't see that  
happening here.  (Part of the trickiness there is that it is a test of the  
test-runner, not running from the source tree.)
Yuya Nishihara - May 16, 2017, 2:29 p.m.
On Mon, 15 May 2017 21:40:24 -0400, Matt Harbison wrote:
> On Mon, 15 May 2017 10:19:19 -0400, Yuya Nishihara <yuya@tcha.org> wrote:
> 
> > On Mon, 15 May 2017 00:08:31 -0400, Matt Harbison wrote:
> >> # HG changeset patch
> >> # User Matt Harbison <matt_harbison@yahoo.com>
> >> # Date 1494808413 14400
> >> #      Sun May 14 20:33:33 2017 -0400
> >> # Node ID 809709930080937b6b56e5cad285798f29a10280
> >> # Parent  024271e90987e5794dab6eb00844467065fae7e4
> >> killdaemons: use posixfile to avoid intermittent unlink errors on  
> >> Windows
> >>
> >> This is the aforementioned fix for the occasional cleanup error with  
> >> #serve
> >> enabled.  There are a handful of tests that neglect to kill the daemons  
> >> they
> >> spawned, and this code is doing a last ditch reap of them.  The test  
> >> that got
> >> flagged was non-deterministic, and I've seen up to 3 fail in the same  
> >> run.
> >>
> >> The problem with trying to import the mercurial module is that while it  
> >> is
> >> available for running the individual *.t files, it is not in sys.path  
> >> for
> >> run-tests.py itself.  I couldn't think of any other way to make this  
> >> work, and
> >> not affect sys.path for the indiviual tests.  (The main source tree  
> >> _is_ in
> >> PYTHONPATH when this is imported from run-tests.py.)
> >>
> >> diff --git a/tests/killdaemons.py b/tests/killdaemons.py
> >> --- a/tests/killdaemons.py
> >> +++ b/tests/killdaemons.py
> >> @@ -7,6 +7,16 @@
> >>  import sys
> >>  import time
> >>
> >> +# PYTHONPATH contains the hg source tree when invoked from  
> >> ./run-tests, but
> >> +# sys.path does not, and importing mercurial fails.  The first import  
> >> works from
> >> +# the .t files without editing the path.
> >> +try:
> >> +    from mercurial.util import posixfile
> >> +except ImportError:
> >> +    srctree =  
> >> os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
> >> +    sys.path.insert(1, srctree)
> >> +    from mercurial.util import posixfile
> >
> > sys.path is global. I slightly prefer moving this hack to run-tests.py if
> > it's okay for run-tests.py to depend on Mercurial modules.
> 
> I'm not sure if the dependency is OK, but I wasn't sure what else to do.   
> (util.posixfile is a wrapper around osutil.posixfile, so it's not like it  
> can be copy/pasted.)

Maybe you can copy from the pure version. The whole class is big, but the
important part would be just a few lines.

If the problem can be mitigated by reading pidfile before killing, we won't
need a posixfile. But I guess the problem wouldn't be that simple.
Matt Harbison - May 17, 2017, 4:58 a.m.
On Tue, 16 May 2017 10:29:45 -0400, Yuya Nishihara <yuya@tcha.org> wrote:

> On Mon, 15 May 2017 21:40:24 -0400, Matt Harbison wrote:
>> On Mon, 15 May 2017 10:19:19 -0400, Yuya Nishihara <yuya@tcha.org>  
>> wrote:
>>
>> > On Mon, 15 May 2017 00:08:31 -0400, Matt Harbison wrote:
>> >> # HG changeset patch
>> >> # User Matt Harbison <matt_harbison@yahoo.com>
>> >> # Date 1494808413 14400
>> >> #      Sun May 14 20:33:33 2017 -0400
>> >> # Node ID 809709930080937b6b56e5cad285798f29a10280
>> >> # Parent  024271e90987e5794dab6eb00844467065fae7e4
>> >> killdaemons: use posixfile to avoid intermittent unlink errors on
>> >> Windows
>> >>
>> >> This is the aforementioned fix for the occasional cleanup error with
>> >> #serve
>> >> enabled.  There are a handful of tests that neglect to kill the  
>> daemons
>> >> they
>> >> spawned, and this code is doing a last ditch reap of them.  The test
>> >> that got
>> >> flagged was non-deterministic, and I've seen up to 3 fail in the same
>> >> run.
>> >>
>> >> The problem with trying to import the mercurial module is that while  
>> it
>> >> is
>> >> available for running the individual *.t files, it is not in sys.path
>> >> for
>> >> run-tests.py itself.  I couldn't think of any other way to make this
>> >> work, and
>> >> not affect sys.path for the indiviual tests.  (The main source tree
>> >> _is_ in
>> >> PYTHONPATH when this is imported from run-tests.py.)
>> >>
>> >> diff --git a/tests/killdaemons.py b/tests/killdaemons.py
>> >> --- a/tests/killdaemons.py
>> >> +++ b/tests/killdaemons.py
>> >> @@ -7,6 +7,16 @@
>> >>  import sys
>> >>  import time
>> >>
>> >> +# PYTHONPATH contains the hg source tree when invoked from
>> >> ./run-tests, but
>> >> +# sys.path does not, and importing mercurial fails.  The first  
>> import
>> >> works from
>> >> +# the .t files without editing the path.
>> >> +try:
>> >> +    from mercurial.util import posixfile
>> >> +except ImportError:
>> >> +    srctree =
>> >> os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
>> >> +    sys.path.insert(1, srctree)
>> >> +    from mercurial.util import posixfile
>> >
>> > sys.path is global. I slightly prefer moving this hack to  
>> run-tests.py if
>> > it's okay for run-tests.py to depend on Mercurial modules.
>>
>> I'm not sure if the dependency is OK, but I wasn't sure what else to do.
>> (util.posixfile is a wrapper around osutil.posixfile, so it's not like  
>> it
>> can be copy/pasted.)
>
> Maybe you can copy from the pure version. The whole class is big, but the
> important part would be just a few lines.
>
> If the problem can be mitigated by reading pidfile before killing, we  
> won't
> need a posixfile. But I guess the problem wouldn't be that simple.

I've got to be missing something obvious.  I thought that as long as  
killdaemons() closes the pid file before unlinking it, that would be all  
that is needed.  I'm not sure why a kill in between is a problem.

That said, I changed the loop to populate a list instead of kill()  
directly.  Then close the file, kill each, and finally unlink.  I only had  
time for a couple test runs, but it ran cleanly.

Patch

diff --git a/tests/killdaemons.py b/tests/killdaemons.py
--- a/tests/killdaemons.py
+++ b/tests/killdaemons.py
@@ -7,6 +7,16 @@ 
 import sys
 import time
 
+# PYTHONPATH contains the hg source tree when invoked from ./run-tests, but
+# sys.path does not, and importing mercurial fails.  The first import works from
+# the .t files without editing the path.
+try:
+    from mercurial.util import posixfile
+except ImportError:
+    srctree = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
+    sys.path.insert(1, srctree)
+    from mercurial.util import posixfile
+
 if os.name =='nt':
     import ctypes
 
@@ -81,18 +91,17 @@ 
         logfn = lambda s: s
     # Kill off any leftover daemon processes
     try:
-        fp = open(pidfile)
-        for line in fp:
-            try:
-                pid = int(line)
-                if pid <= 0:
-                    raise ValueError
-            except ValueError:
-                logfn('# Not killing daemon process %s - invalid pid'
-                      % line.rstrip())
-                continue
-            kill(pid, logfn, tryhard)
-        fp.close()
+        with posixfile(pidfile) as fp:
+            for line in fp:
+                try:
+                    pid = int(line)
+                    if pid <= 0:
+                        raise ValueError
+                except ValueError:
+                    logfn('# Not killing daemon process %s - invalid pid'
+                          % line.rstrip())
+                    continue
+                kill(pid, logfn, tryhard)
         if remove:
             os.unlink(pidfile)
     except IOError: