Code review comment for lp:~allenap/python-pgbouncer/reliable-shutdown

Revision history for this message
Gavin Panella (allenap) wrote :

[...]
> If pgbouncer is a child of the test process, it should die along
> with the process. Don't mess with atexit if you can avoid it. It'd
> be good to give this a manual try though, just in case. You can
> probably just insert a long sleep and issue a manual “kill -segv
> <pid>” to see what happens if the test process dies without any
> python exit handling whatsoever.

SIGSEGV and SIGTERM leave pgbouncer running, bug SIGINT allows
clean-ups to run (because it's a KeyboardInterrupt) so that pgbouncer
gets terminated. I think this is acceptable.

Rob has previously warned (w.r.t. rabbitfixture) not to be too clever
with cleaning stuff up from earlier. He advised something along the
lines of: it's better to let these things go awry and thus draw our
attention to leaks rather than patch things over.

> > > * Why treat the files you open as binary? Take a pidfile, for
> > > instance. I don't suppose it'll ever matter, but isn't that a
> > > text file? Adding the "b" to the open modes seems to confuse
> > > more than help.
> >
> > Just a habit. The Python docs state that using "b" improves
> > portability. Not that it's needed, but it does no harm, and the
> > "features" of text mode are not needed anyway.
>
> AFAIK it only improves portability for files that aren't pure text!

Though there is no different on Linux (afaik), treating every file as
binary reduces surprises on other platforms. Having a distinction
between text and binary reminds me of FTP ASCII mode and what a
horror-show that is.

When writing to a file I rarely use print, for example; I prefer to
use .write(), which means I must think about and insert line-endings
myself. I guess this _might_ tempt me to use text mode for writing a
file, but more often than not I'd simply use Unix line-endings. Only
notepad.exe is likely to have any problem opening files like that.

If I want to read a file line by line that could have any line-ending,
I will either slurp it in and call splitlines(), or use universal
newlines.

In the fixture there are three places where I use binary mode: opening
the output file for pgbouncer, opening the input file (/dev/null) for
pgbouncer, and for reading the PID file.

The first two mimic shell redirections, which must use binary mode (or
else piping tar to gzip would break on Windows).

The PID file is slurped in and immediately stripped (and is expected
to be no more than one line anyway) so line-endings are irrelevant.

« Back to merge proposal