| 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
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.
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.)
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.
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: