tahoe-lafs/trac

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

New issue

Closed

opened 2010-02-03 23:17:06 +00:00 by daira · 10 comments

daira commented

2010-02-03 23:17:06 +00:00

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.

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.

daira added the

labels 2010-02-03 23:17:06 +00:00

daira added this to the 1.7.0 milestone 2010-02-03 23:17:06 +00:00

daira commented

2010-02-03 23:20:54 +00:00

Author

Attachment fix-test-copy-using-filecap-darcspatch.txt (50851 bytes) added

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

**Attachment** fix-test-copy-using-filecap-darcspatch.txt (50851 bytes) added Fix race condition in allmydata.test.test_cli.Cp.test_copy_using_filecap

fix-test-copy-using-filecap-darcspatch.txt

50 KiB

daira commented

2010-02-03 23:25:52 +00:00

Author

Attachment fix-test-copy-using-filecap-diff.txt (1077 bytes) added

Same thing as a unified diff

**Attachment** fix-test-copy-using-filecap-diff.txt (1077 bytes) added Same thing as a unified diff

fix-test-copy-using-filecap-diff.txt

1.1 KiB

warner commented

2010-02-05 04:07:13 +00:00

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()

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() ```

daira commented

2010-02-05 18:36:11 +00:00

Author

Attachment fix-test-copy-using-filecap-2-darcspatch.txt (53375 bytes) added

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

**Attachment** fix-test-copy-using-filecap-2-darcspatch.txt (53375 bytes) added Fix race conditions in allmydata.test.test_cli.Cp.test_copy_using_filecap, and minor cleanup

fix-test-copy-using-filecap-2-darcspatch.txt

52 KiB

daira commented

2010-02-05 20:13:05 +00:00

Author

Assigning to Brian for review.

warner was assigned by daira

2010-02-05 20:13:05 +00:00

daira commented

2010-02-06 01:45:58 +00:00

Author

Attachment fix-test-cli-darcspatch.txt (65072 bytes) added

Previous attachment was missing some of my changes.

**Attachment** fix-test-cli-darcspatch.txt (65072 bytes) added Previous attachment was missing some of my changes.

fix-test-cli-darcspatch.txt

64 KiB

daira commented

2010-02-06 01:49:53 +00:00

Author

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.

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.

daira commented

2010-02-06 01:55:50 +00:00

Author

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.

Replying to [warner](/tahoe-lafs/trac/issues/936#issuecomment-376038): > 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.

daira modified the milestone from 1.7.0 to 1.6.1

2010-02-15 20:21:54 +00:00

zooko commented

2010-02-21 06:00:22 +00:00

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]source:src/allmydata/util/fileutil.py 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.

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]source:src/allmydata/util/fileutil.py are syntactically different from [those same functions in the copy of fileutil in pyutil](http://allmydata.org/trac/pyutil/browser/pyutil/pyutil/fileutil.py?rev=167#L19) 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.