dead code: NamedTemporaryDirectory #2433

New Issue

zooko · 2015-05-26T17:20:01Z

zooko commented

2015-05-26 17:20:01 +00:00

Looking at #2432, I noticed that [NamedTemporaryDirectory]source:trunk/src/allmydata/util/fileutil.py?annotate=blame&rev=143af6151800efd8ecb750e682aa42da254ce5a7#L90 is unused except for [its unit test]source:trunk/src/allmydata/test/test_util.py?annotate=blame&rev=4794666df619fbfd7d36620163b51bda31322773#L434.

Now, [//trac/pyutil/browser/trunk/pyutil/fileutil.py?rev=254#L137 the newer version of NamedTemporaryDirectory] over in pyutil has an added feature — automatically call .close() on children when shutting down — which might be useful on Windows, but probably isn't. In any case, let's remove this older and unused version of NamedTemporaryDirectory from Tahoe-LAFS.

Looking at #2432, I noticed that [NamedTemporaryDirectory]source:trunk/src/allmydata/util/fileutil.py?annotate=blame&rev=143af6151800efd8ecb750e682aa42da254ce5a7#L90 is unused except for [its unit test]source:trunk/src/allmydata/test/test_util.py?annotate=blame&rev=4794666df619fbfd7d36620163b51bda31322773#L434. Now, [//trac/pyutil/browser/trunk/pyutil/fileutil.py?rev=254#L137 the newer version of [NamedTemporaryDirectory](wiki/NamedTemporaryDirectory)] over in pyutil has an added feature — automatically call `.close()` on children when shutting down — which might be useful on Windows, but probably isn't. In any case, let's remove this older and unused version of `NamedTemporaryDirectory` from Tahoe-LAFS.

zooko added the

labels 2015-05-26 17:20:01 +00:00

zooko added this to the undecided milestone 2015-05-26 17:20:01 +00:00

zooko commented

2015-05-26 17:21:48 +00:00

(https://github.com/tahoe-lafs/tahoe-lafs/pull/169)

Zooko <zookog@gmail.com> commented

2015-05-26 18:22:29 +00:00

In /tahoe-lafs/trac-2024-07-25/commit/a9b152780e3c4d875100853477a006edf49964ec:

remove dead code: NamedTemporaryDirectory

fixes #2433

In [/tahoe-lafs/trac-2024-07-25/commit/a9b152780e3c4d875100853477a006edf49964ec](/tahoe-lafs/trac-2024-07-25/commit/a9b152780e3c4d875100853477a006edf49964ec): ``` remove dead code: NamedTemporaryDirectory fixes #2433 ```

tahoe-lafs added the

fixed

label 2015-05-26 18:22:29 +00:00

tahoe-lafs closed this issue

2015-05-26 18:22:29 +00:00

tahoe-lafs modified the milestone from undecided to 1.10.1

2015-05-27 00:00:06 +00:00

tahoe-lafs added

and removed

labels 2015-05-27 00:00:39 +00:00

daira commented

2015-05-27 00:18:12 +00:00

I looked at the implementation of NamedTemporaryFile in pyutil, and I am not convinced that it would work correctly for asynchronous tests. The problem is that the NamedTemporaryFile object may go out of scope and have its *del* method called before all of the callback/errback chains associated with the test have finished (it's possible that the closures of those callbacks/errbacks might hold on to the object for long enough, but that seems error-prone and complicated to reason about).

If we were going to add automatic cleanup, I would want it to only be triggered in the test's cleanup phase. Even that is potentially a problem for diagnosing what happened by looking at the files under _trial_temp in case of a test failure.

I looked at the implementation of `NamedTemporaryFile` in pyutil, and I am not convinced that it would work correctly for asynchronous tests. The problem is that the `NamedTemporaryFile` object may go out of scope and have its `*del*` method called before all of the callback/errback chains associated with the test have finished (it's possible that the closures of those callbacks/errbacks might hold on to the object for long enough, but that seems error-prone and complicated to reason about). If we were going to add automatic cleanup, I would want it to only be triggered in the test's cleanup phase. Even that is potentially a problem for diagnosing what happened by looking at the files under `_trial_temp` in case of a test failure.

Sign in to join this conversation.