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

<snip/>

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

Thanks, I answer below.

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

That's true, we don't. And the way events are used doesn't really allow
that anyway.

    > 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

Right, I did that first, quick-and-dirty to debug but wasn't really
happy with it, then chatting with mgz he objected so I reverted it.

I think both approaches have their pro and cons. You can argue that
'self' should only be used to refer to the object the method is defined
for.

But overall, the three of use having issues with the double use of self
calls for something to be done.

I wanted to add a comment:

  # We use 'request' instead of 'self' below because the test matters
  # more and we need some container to properly set connection_thread

I'll add it before landing.

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

This cannot be done. The handler is instantiated in a place where one
cannot add more parameters.

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

Hehe, I did try that... and realized the tests were more different than
they appear, they really override a different method in the handler and
need to do so.