spurious test failure due to race condition in allmydata.test.test_cli.Cp.test_copy_using_filecap

[davidsarah]

Reported by Ludovic Courtès,

​http://hydra.nixos.org/build/278961/nixlog/14/raw :

allmydata.test.test_cli.Cp.test_copy_using_filecap fails with

allmydata.scripts.common.UnknownAliasError: Unknown alias 'tahoe', please create it with 'tahoe add-alias' or 'tahoe create-alias'.

I think this is because the test creates the "tahoe" alias (line 1000), but does not chain subsequent operations to the resulting Deferred. So, the alias might not have been created by the time it is needed.

[davidsarah]

Fix race condition in allmydata.test.test_cli.Cp.test_copy_using_filecap

[davidsarah]

Same thing as a unified diff

[warner]

looks good. do_cli() definitely needs to be waited upon, since it uses deferToThread(). All sorts of nasty race conditions if that Deferred is ignored.

I would advise also changing the open(fn1, "wb").write(DATA1) at line 15 to explicitly close the filehandle. The file object is not obligated to flush anything until it gets GC'ed, and merely going out of scope is not enough to force that. (in practice I've never seen it behave otherwise, but that probably depends on the platform). Unfortunately the full approach is a bit verbose:

f = open(fn1, "wb")
f.write(...)
f.close()

[davidsarah]

Fix race conditions in allmydata.test.test_cli.Cp.test_copy_using_filecap, and minor cleanup

[davidsarah]

Assigning to Brian for review.

[davidsarah]

Previous attachment was missing some of my changes.

[davidsarah]

Hmm, trac seems to have deleted the description of the patch. It was:

• fix race conditions and missing callback in allmydata.test.test_cli.Cp.test_copy_using_filecap,
• add utilities for one-liner reading and writing of files,
• fix cases in test_cli where files were not being closed after writing.
• (in separate patch), refactor test_cli to use the new utilities in other places.

Note: this now opens files in binary mode where they previously opened in text mode, but I believe that's correct.

The missing callback was _copy_file in test_copy_using_filecap, which led to some of the intended checks in that test not being performed.

[davidsarah]

Replying to warner:

The file object is not obligated to flush anything until it gets GC'ed, and merely going out of scope is not enough to force that. (in practice I've never seen it behave otherwise, but that probably depends on the platform).

I believe it will always behave that way in CPython (due to its use of reference counting), but not in other Python implementations.

[zooko]

I reviewed this patch and it looks right to me. One tiny detail is that the one-liner read and write functions in the copy of fileutil in tahoe-lafs​ are syntactically different from ​those same functions in the copy of fileutil in pyutil which is too bad because it means more diffs next time I manually merge those files. (See #47 (use pyutil as a separate package and contribute src/allmydata/util/* into pyutil).)

I will apply this patch.

[zooko]

fixed by c984a09fe769ba4f and 85a50feeaa0a6150. Thanks!