Opened 2 years ago

Closed 2 years ago

#5509 defect closed fixed (fixed)

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

Reported by: ivank Owned by: exarkun
Priority: normal Milestone:
Component: core Keywords: windows
Cc: ivank-twisted-bugs@… Branch: branches/xmlfile-closing-5509
(diff, github, buildbot, log)
Author: ivank Launchpad Bug:

Description

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.

Attachments (4)

5509-00-load-template-into-memory.patch (866 bytes) - added by ivank 2 years ago.
don't leave a handle open to failure.xhtml
5509-01-XMLFile-FilePath-support.patch (10.6 KB) - added by ivank 2 years ago.
5509-02-XMLFile-FilePath-support.patch (14.1 KB) - added by ivank 2 years ago.
5509-03-XMLFile-FilePath-support.patch (14.2 KB) - added by ivank 2 years ago.

Download all attachments as: .zip

Change History (21)

comment:1 Changed 2 years ago by 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.

comment:2 Changed 2 years ago by ivank

  • Cc ivank-twisted-bugs@… added

comment:3 Changed 2 years ago by ivank

Sorry, by FILE_SHARE_ALL I meant FILE_SHARE_DELETE | FILE_SHARE_READ | FILE_SHARE_WRITE

Changed 2 years ago by ivank

don't leave a handle open to failure.xhtml

comment:4 Changed 2 years ago by ivank

  • Keywords review added

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.

comment:5 Changed 2 years ago by exarkun

  • Keywords review removed
  • Owner set to ivank

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.

comment:6 Changed 2 years ago by 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.
  1. 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.
  1. 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.

comment:7 Changed 2 years ago by ivank

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

comment:8 Changed 2 years ago by ivank

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

  1. XMLFile(FilePath(...)) is shorter than XMLString(FilePath(...).getContent())
  1. XMLFile could later become capable of reloading templates.

Changed 2 years ago by ivank

comment:9 Changed 2 years ago by ivank

  • Keywords review added

I've made these changes:

  1. XMLFile can now take a FilePath.
  1. XMLFile's support for file objects and filenames is deprecated.
  1. I've updated the documentation to pass in FilePaths instead of filenames.
  1. I've added a __repr__ for XMLFile.
  1. FailureElement now passes in a FilePath, solving the original problem.
  1. FailureElement now uses getModule, per glyph's suggestion.
  1. 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.

comment:10 Changed 2 years ago by ivank

  • Owner ivank deleted

comment:11 Changed 2 years ago by itamar

  • Keywords review removed
  • Owner set to ivank

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

Changed 2 years ago by ivank

Changed 2 years ago by ivank

comment:12 Changed 2 years ago by ivank

  • Keywords review added
  • Owner ivank deleted

Thanks for the review.

  1. Okay, I think I've added all the missing docstrings.
  1. Fixed
  1. Fixed
  1. Okay, undone
  1. Fixed
  1. 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).

comment:13 Changed 2 years ago by exarkun

  • Owner set to exarkun
  • Status changed from new to assigned

comment:14 Changed 2 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/xmlfile-closing-5509

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

comment:15 Changed 2 years ago by exarkun

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

refs #5509

comment:16 Changed 2 years ago by exarkun

  • Author changed from exarkun to ivank
  • Keywords review removed

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.

comment:17 Changed 2 years ago by exarkun

  • Resolution set to fixed
  • Status changed from assigned to closed

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

Note: See TracTickets for help on using tickets.