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

[John Arbash Meinel]

...

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

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.


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.


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

John
=:->



-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 254 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060913/044f0d79/attachment.pgp