Code review comment for lp:~vila/bzr/869366-test-server-race

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 10/7/2011 6:34 PM, Vincent Ladeuil wrote:
> Vincent Ladeuil has proposed merging
> lp:~vila/bzr/869366-test-server-race into lp:bzr/2.4.
>
> Requested reviews: bzr-core (bzr-core) Related bugs: Bug #869366 in
> Bazaar: "TestTCPServerInAThread.test_server_crash_while_responding
> random failure" https://bugs.launchpad.net/bzr/+bug/869366
>
> For more details, see:
> https://code.launchpad.net/~vila/bzr/869366-test-server-race/+merge/78634
>
> This is was one of the worst races I had to chase.
>
> For context, this is in bzr-2.4 and only the tests are concerned.
>

 merge: approve

I like the change, and I like that it makes it more deterministic. I
have a couple of comments that might improve clarity, but nothing that
must be done. So feel free to land this, though think about my comments.

John
=:->

...

     def serve(self):
         self.serving = True
- - self.stopped.clear()
         # We are listening and ready to accept connections
         self.started.set()

[comment] This doesn't seem related or necessary. It would allow us to
run 'serve' multiple times, though I suppose we never specifically
need to.

         class FailingDuringResponseHandler(TCPConnectionHandler):

- - def handle_connection(self):
- - req = self.rfile.readline()
- - threading.currentThread().set_sync_event(sync)
+ def handle_connection(request):

^- [comment] changing the 'self' parameter to not be self seems
confusing. I think I preferred your form using:

test = self

Or even better would be to pass 'self' to the
FailingDuringResponseHandler's __init__ constructor, and saving it as
an attribute. This particular change will conflict landing on bzr.dev,
but should be obvious to fix.

The tests are meant to be roughly identical, except the latter case
marks the exception as ignored. Can we move the common code to a
helper to make it purely obvious that this is the case?
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk6POyQACgkQJdeBCYSNAAPEMQCeN5NABM6irUH7z0BRwK9Cr9od
9MwAni0HIw57Kw7XTuWmWbh03xR2P96U
=/GQC
-----END PGP SIGNATURE-----