Can't delete failure.xhtml on Windows because FailureElement keeps a handle open to it

[ivank]

twisted.web.util.FailureElement keeps a handle open to failure.xhtml:

loader = XMLFile(FilePath(__file__).sibling('failure.xhtml').open())

On Windows, this makes it very hard to change the state of a Twisted checkout when I have a twisted.web program running. If I install Twisted, this problem probably makes it impossible to fully install a new Twisted (again, while a program is running).

Process Explorer shows that my program does not keep handles open to other Twisted files, so this problem, despite being a Python open()/Windows issue, should probably be fixed.

[ivank]

Also, unfortunately Python does not provide a way to open a file with the FILE_SHARE_ALL share mode, which would allow the open file to be deleted.

[ivank]

Sorry, by FILE_SHARE_ALL I meant FILE_SHARE_DELETE | FILE_SHARE_READ | FILE_SHARE_WRITE

[ivank]

don't leave a handle open to failure.xhtml

[ivank]

I added a .misc file because this problem hasn't made it into a release, and I doubt anyone else has noticed it in the month it's been there.

[exarkun]

This fix makes me think that XMLFile should never be used, because it will always screw over Windows.

Perhaps instead we should change XMLFile so it's safe to use? Perhaps XMLFile should immediately parse the template given to it and then close the file.

Or perhaps we should get rid of XMLFile, or replace it (or its __init__) with something that takes a FilePath instance instead of a file instance.

[ivank]

Here are my thoughts on three options:

1. Close the file immediately: closing whatever file object the user passes could be surprising and unwanted.

2. Make XMLFile take a FilePath instance that it can read and close: we'd have to support and deprecate the old behavior. Not sure this is a good idea, given XMLFile's marginal utility.

3. Deprecate XMLFile and change all uses of XMLFile to XMLString: this sounds okay to me. It's curious how the docs have a bunch of incorrect uses of XMLFile, passing in a filename instead of a file object.

[ivank]

I just learned that XMLFile can also a string filename, though this is not documented in XMLFile's docstring.

[ivank]

dreid convinced me to not get rid of XMLFile, because:

1. XMLFile(FilePath(...)) is shorter than XMLString(FilePath(...).getContent())

2. XMLFile could later become capable of reloading templates.

[ivank]

I've made these changes:

1. XMLFile can now take a FilePath.

2. XMLFile's support for file objects and filenames is deprecated.

3. I've updated the documentation to pass in FilePaths instead of filenames.

4. I've added a __repr__ for XMLFile.

5. FailureElement now passes in a FilePath, solving the original problem.

6. FailureElement now uses getModule, per glyph's suggestion.

7. FailureElement now loads the template immediately, to prevent it from being out of sync with the Python code it's coupled with.

I did not document the FailureElement changes in NEWS files because they haven't made it into a release.

Please review.

[itamar]

(Review courtesy of JP, who is missing his credentials)

Thanks!

1. Various docstrings need to be added (ie, for all methods)
2. The @param doc in XMLFile.__doc__ was wrong before, and is still wrong. @param is for parameters. This should have been @ivar before. Maybe now it should be an @ivar on the class docstring and a @param in the __init__ docstring (and they're not quite the same).
3. Indentation on the warnings.warn call is weird; align args with the opening ( or put no args on the first line at all and indent by 4 spaces.
4. I think I disagree with the idea of immediately loading the failure template in twisted/web/util.py. If someone replaces the file from under us, do they have any grounds to expect sane behavior? Part of the idea of accepting a path in XMLFile is to be able to _not_ read all of the templates at startup time.
5. In XMLFileReprTests, the test method names can lose the repr substring. They test class already implies this.
6. The flushWarnings use in XMLLoaderTestsMixin is a little fragile. It will collect any warnings emitted during the test. That may be none now, but if some new warning appears later on (eg, from the stdlib xml library) it will make the test fail. I think that passing offendingFunctions=[self.loaderFactory] is a simple fix for this. This both:
   1. Disregards warnings that point at some other call site as the cause
   2. Ensures that the warning emitted by XMLFile correctly points at the code constructing the XMLFile (ie, gets the stacklevel argument correct)

[ivank]

Thanks for the review.

1. Okay, I think I've added all the missing docstrings.

2. Fixed

3. Fixed

4. Okay, undone

5. Fixed

6. Fixed, and confirmed that offendingFunctions= does something

I have also fixed some docstrings, including the XMLString, TagLoader, and _flatsaxParse docstrings. I have also adjusted some vertical whitespace.

Please review (just the latest patch).

[exarkun]

(In [33936]) Branching to 'xmlfile-closing-5509'

[exarkun]

(In [33937]) Apply 5509-03-XMLFile-FilePath-support.patch

refs #5509

[exarkun]

Thanks. Test method docstrings don't need to say "Test that X." They can just say "X." Apart from that, I like the looks of this patch a lot. If  buildbot is similarly satisfied, I'll make those minor doc tweaks and land the change. Thanks again.

[exarkun]

(In [33940]) Merge xmlfile-closing-5509

Author: ivank Reviewer: exarkun Fixes: #5509

Deprecate construction of twisted.web.template.XMLFile instances using a string or a file object and add support for constructing them from a FilePath instance. This also fixes a problem where, when a file object was used, that file object would never be closed.