Code review comment for lp:~jtv/launchpad/bug-717969

> Thanks a *bunch* for tackling this, I realise it's a bit soul-sapping. If it
> helps, know that I have spent most of the day going through your code to make
> sure it's all OK :)

That helps a lot. And I'm well aware that reviewing this stuff is more soul-sapping than writing it is. Thank you for wading through your 1290 lines of personal hell!

> 319 + transaction.commit()
> 320 + with DatabaseTransactionPolicy(read_only=False):
> 321 + library_file =
> getUtility(ILibraryFileAliasSet).create(
> 322 + filename, bytes_written, out_file,
> 323 + contentType=filenameToContentType(filename),
> 324 + restricted=private)
> 325 + transaction.commit()
>
> Should that commit at 319 be an abort()? Or are you trying to make it bomb
> out deliberately if there's data getting written where it should not be?

And there you put your finger on the deciding consideration, which I had totally forgotten in the meantime. Yes, thanks to DatabaseTransactionPolicy, a commit is actually better than an abort. It will reveal violations of the policy in a much more direct and helpful way than an abort would.

(Also, I believe the ORM makes abort slower because it needs to scan the live objects for changes that need undoing).

> 349 transaction.commit()
> 350 + with DatabaseTransactionPolicy(read_only=False):
> 351 + candidate.markAsBuilding(self)
> 352 + transaction.commit()
>
> Same here.
>
> And in fact in quite a few places so I won't list them all.

I went with the commit throughout, which only required 2 changes.

> 586 -from lp.registry.interfaces.pocket import (
> 587 - PackagePublishingPocket,
> 588 - )
> 589 +from lp.registry.interfaces.pocket import PackagePublishingPocket
>
> Why? :)

Automated: utilities/format-new-and-modified-imports.

> 634 - d = method(librarian, slave_status, logger,
> send_notification)
> 635 - return d
> 636 + return None
> 637 + else:
> 638 + return method(librarian, slave_status, logger,
> send_notification)
>
> I'd prefer if you left this with the local "d" variable because it makes it
> clear it's returning a Deferred. This is important because anything that
> reminds the caller is a good thing, especially morons like me who cocked it up
> once :)

You're right. Had I come across this a bit later in the branch, when I had more grey hairs but also more twisted/buildmanager experience, I would have kept the “d” as well. Fixed.

> 945 - self.useFixture(BuilddSlaveTestSetup())
>
> All of the other tests don't need this, and it's pretty slow to set up, which
> is why I recently moved it out of setUp(). Is there a good reason to put it
> back?

Probably a misguided step in changing the test cases over from that weirdly-named Twisted thing to our own TestCaseWithFactory. Fixed.

> Finally - I'd love to see a test that tries to recreate the original bug. I
> think it's actually quite straightforward, I'll try and explain the steps:
>
> * Set up a build connected to a BrokenSlave so that when it's polled it'll
> abort().
> * Create and get a build at the stage where it's inside storeBuildInfo() but
> waiting on the Deferred to fire that returns the log.
> * Fire the reactor up so the outstanding Deferreds are processed - the
> abort() should get called before the log Deferred fires. That will abort all
> the work done in _handleStatus_OK() so is easy to test for.
>
> Hope that makes sense!

It does. Going to work on that next.