[step 10] request: 12 steps towards a high performance server

[Andrew Bennetts]

On Wed, Sep 13, 2006 at 01:10:36PM -0500, John Arbash Meinel wrote:
> 
> ...
> 
> >    http://people.ubuntu.com/~andrew/bzr/start-and-stop_bzr_subprocess/
> >      Add start_bzr_subprocess and stop_bzr_subprocess to allow test code to
> >      continue running concurrently with a subprocess of bzr.  This is useful for
> >      some tests in the next branch...
> 
> I would prefer if it used:
> sys.stdout.write('running\n')
> sys.stdout.flush()
> sys.stdin.readline()
> 
> I realize that they go to the same place, but it is a little more
> explicit this way.

Fair enough.  Changed.

> Also, I would prefer to not use an actual builtin command unless you
> think there is real utility in it. Though I realize you are spawning a
> subprocess, so I'm not sure if we can do anything else.

I'd prefer to avoid this too.  It was the most expedient route.  A slightly more
complex way would be to make a wait-until-signalled.py script and execute that,
but then the start_bzr_subprocess would have to execute something non-bzr, as
Robert points out...

> If you look at the recent patches to bzr.dev (which had to be pulled
> from 0.10-rc2) this is incomplete:
> 
> 
> +        bzr_path = os.path.dirname(os.path.dirname(bzrlib.__file__))+'/bzr'
> +        args = list(args)
> +        process = Popen([sys.executable, bzr_path]+args, stdout=PIPE,
> +                         stderr=PIPE)
> 
> It will fail in an installed copy, because there is no 'bzr' next to
> 'bzrlib'. In that code we switch to 'sys.argv[0]' if the bzr_path
> doesn't exist. We could refactor it into a helper, which returns the
> 'optimal bzr' to run.
> 
> But as is, these tests will fail in an installed bzr. And while we
> aren't as strict for an installed bzr, we would like them to pass if it
> isn't too difficult.

I've refactored run_bzr_subprocess to use start_bzr_subprocess and
finish_bzr_subprocess, keeping the improvements made to run_bzr_subprocess.  So
now they should work equally well.

> +        if send_signal is not None:
> +            os.kill(process.pid, send_signal)
> 
> There is no 'os.kill()' on Windows. So we need to have workarounds if we
> expect that to work. Probably tests that want to 'kill()' just need to
> be skipped.

As Robert suggested, I've added a skip_if_plan_to_signal flag to
start_bzr_subprocess.  It will raise TestSkipped if os.kill doesn't exist.

> Also, we need to be careful that core bzr code does not expect to be
> able to kill() a running process. I'm not sure what you are trying to do
> with it. I support using advanced functionality when it helps (I don't
> think we need to always use lowest-common-denominator). But we need to
> avoid a few things to stay compatible. (Like preexec_fn, os.kill, etc).

We're not expected that; we just want a bit more control over how we interact
with a subprocess we're testing.

> As is, any test that uses finish_bzr_subprocess() would raise an
> AttributeError, and then things would get wonky. (You either end up with
> a stalled process, or you get something like EPIPE).
> 
> Also, if you are planning to start a process, talk to it for a bit, and
> then call finish_bzr_subprocess(), I didn't think process.communicate()
> was really allowed after you had manually read/written bytes.

No, I see nothing in the documentation about communicate being not allowed after
using the pipes directly.  It just says it will write to the stdin, and read
from stdout/err until process completion.

> So I would block on it until we sort out win32 compatibility. But
> otherwise the patch seems fine.

The skip_if_plan_to_signal parameter should address that, so I'll ask Robert to
merge this.

-Andrew.