[PATCH stable?] run-test: ensure the test port are available before launching test

Pierre-Yves David pierre-yves.david at ens-lyon.org
Thu May 7 22:00:51 CDT 2015



On 05/07/2015 07:37 PM, Gregory Szorc wrote:
> On Thu, May 7, 2015 at 6:21 PM, Pierre-Yves David
> <pierre-yves.david at ens-lyon.org <mailto:pierre-yves.david at ens-lyon.org>>
> wrote:
>
>     # HG changeset patch
>     # User Pierre-Yves David <pierre-yves.david at fb.com
>     <mailto:pierre-yves.david at fb.com>>
>     # Date 1431044040 25200
>     #      Thu May 07 17:14:00 2015 -0700
>     # Node ID c25b2adb3664cd3c488e2c53aab0c64100d40af7
>     # Parent  17ba4ccd20b48511b3d06ab47fb1b2faf31410d7
>     run-test: ensure the test port are available before launching test
>
>     I'm running into systematic issue because there is always some port
>     taken in the
>     1500 wide range of port used by the test (3 for each test file).
>
>     diff --git a/tests/run-tests.py b/tests/run-tests.py
>     --- a/tests/run-tests.py
>     +++ b/tests/run-tests.py
>     @@ -47,10 +47,11 @@ import errno
>       import optparse
>       import os
>       import shutil
>       import subprocess
>       import signal
>     +import socket
>       import sys
>       import tempfile
>       import time
>       import random
>       import re
>     @@ -76,10 +77,22 @@ processlock = threading.Lock()
>       if sys.version_info < (2, 5):
>           subprocess._cleanup = lambda: None
>
>       wifexited = getattr(os, "WIFEXITED", lambda x: False)
>
>     +def checkportisavailable(port):
>     +    """return true is nobody seem a port seems free to bind on
>     localhost"""
>     +    try:
>     +        s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
>     +        s.bind(('localhost', port))
>     +        s.close()
>     +        return True
>     +    except socket.error, exc:
>     +        if not exc.errno == errno.EADDRINUSE:
>     +            raise
>     +        return False
>     +
>       closefds = os.name <http://os.name> == 'posix'
>       def Popen4(cmd, wd, timeout, env=None):
>           processlock.acquire()
>           p = subprocess.Popen(cmd, shell=True, bufsize=-1, cwd=wd, env=env,
>                                close_fds=closefds,
>     @@ -1601,10 +1614,12 @@ class TestRunner(object):
>               self._tmpbinddir = None
>               self._pythondir = None
>               self._coveragefile = None
>               self._createdfiles = []
>               self._hgpath = None
>     +        self._portoffset = 0
>     +        self._ports = {}
>
>           def run(self, args, parser=None):
>               """Run the test suite."""
>               oldmask = os.umask(022)
>               try:
>     @@ -1805,10 +1820,28 @@ class TestRunner(object):
>               if failed:
>                   return 1
>               if warned:
>                   return 80
>
>     +    def _getport(self, count):
>     +        port = self._ports.get(count) # do we have a cached entry?
>     +        if port is None:
>     +            port = self.options.port + self._portoffset
>     +            portneeded = 3
>     +            # above 100 tries we just give up and let test reports
>     failure
>     +            for tries in xrange(100):
>     +                allfree = True
>     +                for idx in xrange(portneeded):
>     +                    if not checkportisavailable(port + idx):
>
>     +                        allfree = False
>     +                        break
>     +                self._portoffset += portneeded
>     +                if allfree:
>     +                    break
>     +            self._ports[count] = port
>     +        return port
>     +
>
>
> This is hard to read. Part of me thinks there is a race condition here.
> The offset management in particular has me worried.

The test (therefore this function) is called in the main thread before 
we start using thread so no race condition should happen.

> Ideally, tests would bind to port 0 and let the OS choose an existing
> port. That's the only guaranteed way to prevent race conditions. But
> then we'd need to parse port numbers out of command output so they could
> be used in the rest of tests and we'd need a mechanism to register
> replacements in test output. Scope boat.

Would could have variable management inside the test, but I expect 
headache from it.

> If you are going to rewrite port selection, I'd almost prefer having the
> test harness choose random numbers between 1024 and 65535, attempt to
> bind, and assign that. This seems simple and effective. There is no need
> to maintain state: just try binding in a loop until it works. Is this
> solution too simple? Is user control over the port range really that
> important? We're not talking about a production service here...

Choosing random number for each test does not guarantee test will not 
choose overlapping port number. The math says there is more than 99% 
change it will happen.

>
>           def _gettest(self, test, count):
>               """Obtain a Test by looking at its filename.
>
>               Returns a Test instance. The Test may not be runnable if
>     it doesn't
>               map to a known type.
>     @@ -1826,11 +1859,11 @@ class TestRunner(object):
>
>               t = testcls(refpath, tmpdir,
>                           keeptmpdir=self.options.keep_tmpdir,
>                           debug=self.options.debug,
>                           timeout=self.options.timeout,
>     -                    startport=self.options.port + count * 3,
>     +                    startport=self._getport(count),
>                           extraconfigopts=self.options.extra_config_opt,
>                           py3kwarnings=self.options.py3k_warnings,
>                           shell=self.options.shell)
>               t.should_reload = True
>               return t
>     _______________________________________________
>     Mercurial-devel mailing list
>     Mercurial-devel at selenic.com <mailto:Mercurial-devel at selenic.com>
>     http://selenic.com/mailman/listinfo/mercurial-devel
>
>

-- 
Pierre-Yves David


More information about the Mercurial-devel mailing list