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

[Robert Collins]

On Wed, 2006-09-13 at 13:10 -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.

> 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.

It seemed a better compromise than overriding the path and creating a
different executable on the fly - after all we want to test *bzr*'s
behaviour, and a different executable would not be that any more.

> 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 think the reason Andrew had not sent bundles is because he has not
merged bzr.dev just yet - specifically in this branch the plan was to
turn run_bzr_subprocess into a call to start_bzr_subprocess followed by
finish_bzr_subprocess - and incorporate the fixes from bzr.dev.

> 
> +        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.
> 
> 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).
> 
> 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.
> 
> So I would block on it until we sort out win32 compatibility. But
> otherwise the patch seems fine.

We should do this:
 - add a 'plan_to_signal' option to start_bzr_subprocess
 - if that is true, on windows, raise an error, or perhaps skip(). I'm
not sure whether skipping is better or an explicit error.

Either way, we have blackbox tests for bzr serve, which will block and
if we can't kill on windows using stock python, then we need to skip the
tests. (Win32 does support killing processes FWIW).

-Rob



-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 191 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060914/a7ebf620/attachment.pgp