launchpad-reviewers team mailing list archive

[Stuart Bishop <stuart.bishop@xxxxxxxxxxxxx>]

Stuart Bishop has proposed merging lp:~stub/python-pgbouncer/devel into lp:python-pgbouncer.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #841848 in Python PGBouncer: "No support for unix_socket_dir"
  https://bugs.launchpad.net/python-pgbouncer/+bug/841848
  Bug #841851 in Python PGBouncer: "pgbouncer executable must be found in $PATH"
  https://bugs.launchpad.net/python-pgbouncer/+bug/841851
  Bug #846236 in Python PGBouncer: "pgbouncer fixture breaking in buildbot"
  https://bugs.launchpad.net/python-pgbouncer/+bug/846236

For more details, see:
https://code.launchpad.net/~stub/python-pgbouncer/devel/+merge/74885

Avoid race conditions with pidfile. In fact, ignore the pidfile entirely by running pgbouncer in foreground mode.

Consider the process available when it is accepting socket connections, rather than when it happens to have written its pidfile.
-- 
https://code.launchpad.net/~stub/python-pgbouncer/devel/+merge/74885
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stub/python-pgbouncer/devel into lp:python-pgbouncer.

=== modified file 'pgbouncer/__init__.py'
--- pgbouncer/__init__.py	2011-07-18 03:31:27 +0000
+++ pgbouncer/__init__.py	2011-09-10 10:39:23 +0000
@@ -25,7 +25,7 @@
 # established at this point, and setup.py will use a version of next-$(revno).
 # If the releaselevel is 'final', then the tarball will be major.minor.micro.
 # Otherwise it is major.minor.micro~$(revno).
-__version__ = (0, 0, 1, 'beta', 0)
+__version__ = (0, 0, 4, 'beta', 0)
 
 __all__ = [
     'PGBouncerFixture',

=== modified file 'pgbouncer/fixture.py'
--- pgbouncer/fixture.py	2011-09-05 15:33:21 +0000
+++ pgbouncer/fixture.py	2011-09-10 10:39:23 +0000
@@ -26,6 +26,9 @@
 
 from fixtures import Fixture, TempDir
 
+YIELD_TIME = 0.02  # How much time to sleep in busy loops.
+PROCESS_TIMEOUT = 5  # How long we wait for pgbouncer to startup and shutdown.
+
 def _allocate_ports(n=1):
     """Allocate `n` unused ports.
 
@@ -76,7 +79,6 @@
         self.port = _allocate_ports()[0]
         self.configdir = self.useFixture(TempDir())
         self.auth_type = 'trust'
-        self.process_pid = None
         self.setUpConf()
         self.start()
 
@@ -86,7 +88,6 @@
         self.authpath = os.path.join(self.configdir.path, 'users.txt')
         self.logpath = os.path.join(self.configdir.path, 'pgbouncer.log')
         self.pidpath = os.path.join(self.configdir.path, 'pgbouncer.pid')
-        self.outputpath = os.path.join(self.configdir.path, 'output')
         with open(self.inipath, 'wt') as inifile:
             inifile.write('[databases]\n')
             for item in self.databases.items():
@@ -111,18 +112,19 @@
                 authfile.write('"%s" "%s"\n' % user_creds)
 
     def stop(self):
-        if self.process_pid is None:
+        if self.process is None:
             return
-        os.kill(self.process_pid, signal.SIGTERM)
-        # Wait for the shutdown to occur
-        start = time.time()
-        stop = start + 5.0
-        while time.time() < stop:
-            if not os.path.isfile(self.pidpath):
-                self.process_pid = None
-                return
-        # If its not going away, we might want to raise an error, but for now
-        # it seems reliable.
+        try:
+            self.process.terminate()
+            # Wait for the shutdown to occur
+            start = time.time()
+            stop = start + PROCESS_TIMEOUT
+            while self.process.poll() is None and time.time() < stop:
+                time.sleep(YIELD_TIME)
+        finally:
+            # If its not going away, we might want to raise an error,
+            # but for now it seems reliable.
+            self.process = None
 
     def start(self):
         self.addCleanup(self.stop)
@@ -137,16 +139,24 @@
                 env = os.environ.copy()
                 env['PATH'] = ':'.join(path)
 
-        outputfile = open(self.outputpath, 'wt')
         self.process = subprocess.Popen(
-            [self.pgbouncer, '-d', self.inipath], env=env,
-            stdout=outputfile.fileno(), stderr=outputfile.fileno())
-        self.process.communicate()
-        # Wait up to 5 seconds for the pid file to exist
-        start = time.time()
-        stop = start + 5.0
-        while time.time() < stop:
-            if os.path.isfile(self.pidpath):
-                self.process_pid = int(file(self.pidpath, 'rt').read())
+            [self.pgbouncer, '-q', self.inipath], env=env,
+            stdout=subprocess.PIPE, stderr=subprocess.STDOUT,
+            stdin=subprocess.PIPE)
+        self.process.stdin.close()
+        self.process.stdout.close()
+
+        # Wait up to 5 seconds for pgbouncer to launch start responding, or
+        # raise an exception.
+        now = time.time()
+        stop = now + PROCESS_TIMEOUT
+        while True:
+            try:
+                test_socket = socket.create_connection(
+                    ('127.0.0.1', self.port), stop - now)
                 return
-        raise Exception('timeout waiting for pgbouncer to create pid file')
+            except Exception:
+                now = time.time()
+                if now >= stop:
+                    raise
+            time.sleep(YIELD_TIME)

=== modified file 'setup.py'
--- setup.py	2011-07-25 08:33:53 +0000
+++ setup.py	2011-09-10 10:39:23 +0000
@@ -22,7 +22,7 @@
 description = file(os.path.join(os.path.dirname(__file__), 'README'), 'rb').read()
 
 setup(name="pgbouncer",
-      version="0.0.2",
+      version="0.0.4",
       description="Fixture to bring up temporary pgbouncer instance.",
       long_description=description,
       maintainer="Launchpad Developers",
@@ -31,7 +31,7 @@
       packages=['pgbouncer'],
       package_dir = {'':'.'},
       classifiers = [
-          'Development Status :: 2 - Pre-Alpha',
+          'Development Status :: 4 - Beta',
           'Intended Audience :: Developers',
           'License :: OSI Approved :: GNU Affero General Public License v3',
           'Operating System :: OS Independent',