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

[Gregory Szorc]

On Thu, May 7, 2015 at 6:21 PM, Pierre-Yves David <
pierre-yves.david at ens-lyon.org> wrote:

> # HG changeset patch
> # User Pierre-Yves David <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 == '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.

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.

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


>      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
> http://selenic.com/mailman/listinfo/mercurial-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20150507/c0443a74/attachment.html>