Merge lp:~stub/python-pgbouncer/devel into lp:python-pgbouncer

[Stuart Bishop]

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.

[Jeroen T. Vermeulen (jtv)]

Much better approach I think, thanks.

There's a small wart still, as far as I can see: a timeout seems likely to manifest itself in the form of a nonpositive timeout value being passed to the connection call.

That will still raise a timeout-related exception when that happens so it's not altogether wrong, but it might confuse post-mortem analysis. In my quick experiments I get “socket.error: [Errno 115] Operation now in progress” when the timeout is zero, and “ValueError: Timeout value out of range” when it's negative.

review: Approve

[Stuart Bishop (stub)]

On Sat, Sep 10, 2011 at 9:49 PM, Jeroen T. Vermeulen <email address hidden> wrote:
> Review: Approve
>
> Much better approach I think, thanks.
>
> There's a small wart still, as far as I can see: a timeout seems likely to manifest itself in the form of a nonpositive timeout value being passed to the connection call.

I don't see how a nonpositive timeout can be passed to the connection
call unless someone sets the constance to <= 0. The loop is exited
with a 'raise' if we grab a new value of 'now' and find the timeout
has been hit.

--
Stuart Bishop <email address hidden>
http://www.stuartbishop.net/

[Robert Collins (lifeless)]

Oh, one more thing to check - without -d, pgbouncer may write to
stdout/stderr. If it does this it will deadlock eventually.

[Stuart Bishop (stub)]

The -q option should take care of stdout/stderr noise. We might lose console errors if the process fails to start up for some reason (missing dynamic libraries or similar).

[Stuart Bishop (stub)]

We could pass shell=True and redirect stdout and stderr to /dev/null to be sure, but I don't think we need to bother.

[lp:~stub/python-pgbouncer/devel]

10. By Stuart Bishop on 2011-09-12

Let pgbouncer emit noise rather than hide the failure, avoiding potential hangs

[Stuart Bishop (stub)]

I'm no longer redirecting stdout/stderr to a pipe and closing it. It should be silent, and if it isn't we should here about it.

[Gavin Panella (allenap)]

Fwiw, the following would work too:

    with open(os.devnull, "r+b") as devnull:
        process = subprocess.Popen(
            ..., stdin=devnull, stdout=devnull, stderr=devnull)

[Robert Collins (lifeless)]

On Tue, Sep 13, 2011 at 3:50 AM, Stuart Bishop
<email address hidden> wrote:
> I'm no longer redirecting stdout/stderr to a pipe and closing it. It should be silent, and if it isn't we should here about it.

That will break lxc. Please put it back. Details are in the various
merges I did to LP itself a month back.

[Robert Collins (lifeless)]

Less abruptly, I've mailed the -dev list the (hard) lesson learnt
there. And to avoid creating extra work for you, I've rolled this
back, proposed a narrower fix, got that reviewed, released and its now
playing in EC2 to upgrade LP to 0.0.5.

[Stuart Bishop (stub)]

On Tue, Sep 13, 2011 at 7:28 AM, Robert Collins
<email address hidden> wrote:
> Less abruptly, I've mailed the -dev list the (hard) lesson learnt
> there. And to avoid creating extra work for you, I've rolled this
> back, proposed a narrower fix, got that reviewed, released and its now
> playing in EC2 to upgrade LP to 0.0.5.

So this version is missing the '\n' detection to ensure the pid number
has actually been flushed as expected, and also doesn't help with the
race condition where starting up enough to write the pid file doesn't
mean the process is bound and accepting connections. I think what I
had except for my final commit would have been better, or using
os.devnull if we don't trust -q.

--
Stuart Bishop <email address hidden>
http://www.stuartbishop.net/

[Robert Collins (lifeless)]

On Tue, Sep 13, 2011 at 3:19 PM, Stuart Bishop
<email address hidden> wrote:
> On Tue, Sep 13, 2011 at 7:28 AM, Robert Collins
> <email address hidden> wrote:
>> Less abruptly, I've mailed the -dev list the (hard) lesson learnt
>> there. And to avoid creating extra work for you, I've rolled this
>> back, proposed a narrower fix, got that reviewed, released and its now
>> playing in EC2 to upgrade LP to 0.0.5.
>
> So this version is missing the '\n' detection to ensure the pid number
> has actually been flushed as expected, and also doesn't help with the
> race condition where starting up enough to write the pid file doesn't
> mean the process is bound and accepting connections. I think what I
> had except for my final commit would have been better, or using
> os.devnull if we don't trust -q.

We can tweak further. The pid file doesn't have the \n - but as *all
of Ubuntu* depends on sane behaviour for pid files I don't think we
need a higher standard - if pgbouncer really writes the pid contents
byte-and-flush-at-a-time, then it is what needs fixing, not out
fixture.

On the not listening side, I haven't checked the code - if there is a
race there as well then yes we need to copy that part of your patch
forward as well.

os.devnull would discard what output it has - using a temp file to get
its output is simple and robust. If it writes that its started to that
file we could check there, or just probe as you did - thats a good
thing to do.

FWIW the 0.0.4 version broke jenkins on the librarian patch, and 0.0.5
works again - but we haven't drilled in enough to know if its a flakey
test or actual fallout from pgbouncer fixture changes.

[Stuart Bishop (stub)]

We have one failure in
lp.services.job.tests.test_runner.TestTwistedJobRunner.test_timeout_short
which doesn't go near the fixture, and another in the new Librarian
disconnection tests that might be a genuine failure (got a 200
response when 503 expected, so either connections did not get routed
through pgbouncer or there is a problem in the fixture stop() method).

--
Stuart Bishop <email address hidden>
http://www.stuartbishop.net/

=== modified file 'pgbouncer/__init__.py'
--- pgbouncer/__init__.py	2011-07-18 03:31:27 +0000
+++ pgbouncer/__init__.py	2011-09-12 15:49:32 +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-12 15:49:32 +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,22 @@
                 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,
+            stderr=subprocess.STDOUT, stdin=subprocess.PIPE)
+        self.process.stdin.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-12 15:49:32 +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',