Code review comment for lp:~stub/python-pgbouncer/devel

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.