Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tempfile.NamedTemporaryFile not particularly useful on Windows #58451

Open
dabrahams mannequin opened this issue Mar 10, 2012 · 83 comments
Open

tempfile.NamedTemporaryFile not particularly useful on Windows #58451

dabrahams mannequin opened this issue Mar 10, 2012 · 83 comments
Labels
3.10 OS-windows stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@dabrahams
Copy link
Mannequin

dabrahams mannequin commented Mar 10, 2012

BPO 14243
Nosy @pfmoore, @jaraco, @ncoghlan, @pitrou, @ericvsmith, @tjguk, @jwilk, @merwok, @bitdancer, @njsmith, @methane, @dabrahams, @ethanfurman, @sorcio, @vadmium, @zware, @dlenski, @eryksun, @zooba, @ethanhs, @Ev2geny
PRs
  • bpo-14243: Optionally delete NamedTemporaryFile on content manager exit but not on file close #22431
  • gh-58451: Add optional delete_on_close parameter to NamedTemporaryFile #97015
  • Files
  • ntempfile.py
  • share.py
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = None
    created_at = <Date 2012-03-10.02:14:08.216>
    labels = ['type-feature', 'library', '3.10', 'OS-windows']
    title = 'tempfile.NamedTemporaryFile not particularly useful on Windows'
    updated_at = <Date 2021-05-01.01:32:51.852>
    user = 'https://github.com/dabrahams'

    bugs.python.org fields:

    activity = <Date 2021-05-01.01:32:51.852>
    actor = 'methane'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Library (Lib)', 'Windows']
    creation = <Date 2012-03-10.02:14:08.216>
    creator = 'dabrahams'
    dependencies = []
    files = ['26215', '26229']
    hgrepos = []
    issue_num = 14243
    keywords = ['patch']
    message_count = 83.0
    messages = ['155278', '155309', '155316', '155317', '155333', '155365', '155374', '155375', '155457', '157639', '157925', '157927', '157946', '157947', '157948', '157949', '157952', '164358', '164369', '164375', '164392', '164433', '164487', '164495', '164496', '164497', '164503', '164504', '164505', '164509', '164514', '164617', '184053', '184056', '184088', '228371', '244762', '288473', '288504', '288520', '288521', '288538', '288540', '376593', '376645', '376649', '376656', '376659', '376662', '376663', '376664', '376665', '376666', '376684', '376697', '376702', '377580', '390498', '390805', '390806', '390810', '390814', '390888', '390894', '390899', '390903', '390906', '390908', '390911', '392252', '392253', '392257', '392442', '392447', '392470', '392479', '392480', '392483', '392484', '392505', '392511', '392515', '392559']
    nosy_count = 26.0
    nosy_names = ['paul.moore', 'jaraco', 'ncoghlan', 'pitrou', 'eric.smith', 'tim.golden', 'jwilk', 'eric.araujo', 'r.david.murray', 'njs', 'methane', 'dabrahams', 'ethan.furman', 'davide.rizzo', 'sbt', 'Gabi.Davar', 'martin.panter', 'piotr.dobrogost', 'zach.ware', 'dlenski', 'eryksun', 'steve.dower', 'Carl Osterwisch', 'ethan smith', 'ev2geny', 'chary314']
    pr_nums = ['22431']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue14243'
    versions = ['Python 3.10']

    @dabrahams
    Copy link
    Mannequin Author

    dabrahams mannequin commented Mar 10, 2012

    NamedTemporaryFile is too hard to use portably when you need to open the file by name after writing it. To do that, you need to close the file first (on Windows), which means you have to pass delete=False, which in turn means that you get no help in cleaning up the actual file resource, which as you can see from the code in tempfile.py is devilishly hard to do correctly. The fact that it's different on posix (you can open the file for reading by name without closing it first) makes this problem worse. What we really need for this use-case is a way to say, "delete on __del__ but not on close()."

    @dabrahams dabrahams mannequin added the stdlib Python modules in the Lib dir label Mar 10, 2012
    @pitrou
    Copy link
    Member

    pitrou commented Mar 10, 2012

    This is quite silly indeed, and is due to the use of O_TEMPORARY in the file creation flags.

    @pitrou pitrou added the type-bug An unexpected behavior, bug, or error label Mar 10, 2012
    @pitrou pitrou changed the title NamedTemporaryFile usability request NamedTemporaryFile unusable under Windows Mar 10, 2012
    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Mar 10, 2012

    What's the proposal here? If delete is True, close() must delete the file. It is not acceptable for close() and __del__() to behave differently.

    OTOH, if the proposal is merely to change the way the file is opened on Windows so that it can be opened again without closing it first, that sounds fine.

    @pitrou
    Copy link
    Member

    pitrou commented Mar 10, 2012

    OTOH, if the proposal is merely to change the way the file is opened
    on Windows so that it can be opened again without closing it first,
    that sounds fine.

    That would be my proposal. It probably needs getting rid of O_TEMPORARY,
    exposing CreateFile and _open_osfhandle, and using the FILE_SHARE_DELETE
    open mode.

    @dabrahams
    Copy link
    Mannequin Author

    dabrahams mannequin commented Mar 10, 2012

    I disagree that it's unacceptable for close() and __del__() to behave differently. The acceptable difference would be that __del__() closes (if necessary) /and/ deletes the file on disk, while close() merely closes the file.

    If you can in fact "change the way the file is opened on Windows so that it can be opened again without closing it first," that would be fine with me. It isn't clear to me that Windows supports that option, but I'm not an expert.

    Another possibility, of course, is something like what's implemented in:
    dabrahams/zeroinstall@d76de03#L3R44
    (an optional argument to close() that prevents deletion).

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Mar 11, 2012

    The whole point of close() methods is to offer deterministic resource management to applications that need it. Pointing out to applications when they're relying on CPython's refcounting for prompt resource cleanup is why many of the standard types now trigger ResourceWarning for any application that relies on the GC to clean up such external resources in __del__.

    So, no, we're not going to back away from the explicit guarantee in the NamedTemporaryFile docs: "If delete is true (the default), the file is deleted as soon as it is closed." (Especially since doing so would also breach backward compatibility guarantees)

    However, you're right that the exclusive read lock in the current implementation makes the default behaviour of NamedTemporaryFile significantly less useful on Windows than it is on POSIX systems, so the implementation should be changed to behave more like the POSIX variant.

    @dabrahams
    Copy link
    Mannequin Author

    dabrahams mannequin commented Mar 11, 2012

    If file.close() "offers deterministic resource management," then you have to consider the file's open/closed state to be a resource separate from its existence. A NamedTemporaryFile whose close() deterministically managed the open/closed state but not the existence of the file would be consistent with file. That said, I understand the move toward deprecating (in the informal sense) cleanups that rely on GC.

    I'm not suggesting breaking backward compatibility, either. I'm suggesting that it might make sense to allow an explicit close-without-delete as an /extension/ of the current interface. Given the move away from GC-cleanups, you'd probably want an explicit unlink() method as well in that case.

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Mar 11, 2012

    Dave, decoupling the lifecycle of the created file from the object that created it is exactly what delete=False already covers. The complicated dance in NamedTemporaryFile is only to make *del* work a bit more reliably during process shutdown (due to some messy internal problems with what CPython is doing at that point). If you're doing deterministic cleanup (even via atexit), you don't need any of that - you can just use os.unlink().

    @dabrahams
    Copy link
    Mannequin Author

    dabrahams mannequin commented Mar 12, 2012

    Nick, not to belabor this, but I guess you don't understand the use-case in question very well, or you'd see that delete=False doesn't cover it.

    The use case is this: I have to write a test for a function that takes a filename as a parameter and opens and reads from the file with that name. The test should conjure up an appropriate file, call the function, check the results, and clean up the file afterwards. It doesn't matter when the file gets cleaned up, as long as it is cleaned up "eventually." Having to explicitly delete the file is exactly the kind of boilerplate one wants to avoid in situations like this.

    Even if Windows allows a file to be opened for reading (in some circumstances) when it is already open for writing, it isn't hard to imagine that Python might someday have to support an OS that didn't allow it under any circumstances. It is also a bit perverse to have to keep the file open for writing after you're definitively done writing it, just to prevent it from being deleted prematurely. I can understand most of the arguments you make against close-without-delete, except those (like the above) that seem to come from a "you shouldn't want that; it's just wrong" stance.

    @bitdancer
    Copy link
    Member

    bitdancer commented Apr 6, 2012

    See bpo-14514 for an alternate proposal to solve this. I did search before I opened that issue, but search is currently somewhat broken and I did not find this issue. I'm not marking it as a dup because my proposal is really a new feature.

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Apr 10, 2012

    I agree we need to add something here to better support the idiom where the "close" and "delete" operations on a NamedTemporaryFile are decoupled without the delete becoming a completely independent call to os.unlink().

    I agree with RDM's proposal in bpo-14514 that the replacement should be "delete on __exit__ but not on close". As with generator context managers, I'd also add in the "last ditch" cleanup behaviour in __del__.

    Converting the issue to a feature request for 3.3 - there's no bug here, just an interaction with Windows that makes the existing behavioural options inconvenient.

    After all, you can currently get deterministic cleanup (with a __del__ fallback) via:

      @contextmanager
      def named_temp(name):
        f = NamedTemporaryFile(name, delete=False)
        try:
            yield f
        finally:
            try:
                os.unlink(name)
            except OSError:
                pass

    You need to be careful to make sure you keep the CM alive (or it will delete the file behind your back), but the idiom RDM described in the other issues handles that for you:

      with named_temp(fname) as f:
         data = "Data\n"
         f.write(data)
         f.close() # Windows compatibility
         with open(fname) as f:
             self.assertEqual(f.read(), data)

    As far as the API goes, I'm inclined to make a CM with the above behavour available as a new class method on NamedTemporaryFile:

      with NamedTemporaryFile.delete_after(fname) as f:
          # As per the workaround

    @ncoghlan ncoghlan changed the title NamedTemporaryFile unusable under Windows tempfile.NamedTemporaryFile not particularly useful on Windows Apr 10, 2012
    @ncoghlan ncoghlan added type-feature A feature request or enhancement and removed type-bug An unexpected behavior, bug, or error labels Apr 10, 2012
    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Apr 10, 2012

    Although, for the stdlib version, I wouldn't suppress the OS Error (I'd follow what we currently do for TemporaryDirectory)

    @bitdancer
    Copy link
    Member

    bitdancer commented Apr 10, 2012

    "delete_after" what? I know it is somewhat implicit in the fact that it is a context manager call, but that is not the only context the method name will be seen in. (eg: 'dir' list of methods, doc index, etc). Even as a context manager my first thought in reading it was "delete after what?", and then I went, "oh, right".

    How about "delete_on_exit"?

    @bitdancer
    Copy link
    Member

    bitdancer commented Apr 10, 2012

    By the way, I still think it would be nicer just to have the context manager work as expected with delete=True (ie: doesn't delete until the end of the context manager, whether the file is closed or not). I'm OK with being voted down on that, though.

    @pitrou
    Copy link
    Member

    pitrou commented Apr 10, 2012

    By the way, I still think it would be nicer just to have the context
    manager work as expected with delete=True (ie: doesn't delete until
    the end of the context manager, whether the file is closed or not).
    I'm OK with being voted down on that, though.

    Indeed, the current behaviour under Windows seems to be kind of a
    nuisance, and having to call a separate method doesn't sound very
    user-friendly.

    @jaraco
    Copy link
    Member

    jaraco commented Apr 10, 2012

    I agree. If the primary usage of the class does not work well on Windows, developers will continue to write code using the primary usage because it works on their unix system, and it will continue to cause failures when run on windows. Because Python should run cross-platform, I consider this a bug in the implementation and would prefer it be adapted such that the primary use case works well on all major platforms.

    If there is a separate class method for different behavior, it should be for the specialized behavior, not for the preferred, portable behavior.

    I recognize there are backward-compatibility issues here, so maybe it's necessary to deprecate NamedTemporaryFile in favor of a replacement.

    @jaraco jaraco changed the title tempfile.NamedTemporaryFile not particularly useful on Windows tempfile.NamedTemporaryFile not particularly useful on Windows Apr 10, 2012
    @bitdancer
    Copy link
    Member

    bitdancer commented Apr 10, 2012

    Well, fixing NamedTemporaryFile in either of the ways we've discussed isn't going to fix people writing non-portable code. A unix coder isn't necessarily going to close the file before reading it. However, it would at least significantly increase the odds that the code would be portable, while the current situation *ensures* that the code is not portable.

    @tjguk
    Copy link
    Member

    tjguk commented Jun 29, 2012

    Daniel. If you have any interest in this issue, would you mind
    summarising the state of affairs, please? I have no direct interest in
    the result but I'm happy to commit a patch or even to work one up if
    somone can come up with a single, concrete suggestion.

    @dlenski
    Copy link
    Mannequin

    dlenski mannequin commented Jun 30, 2012

    Tim Golden,
    My preferred solution would be to replace the binary delete argument of the current NamedTemporaryFile implementation with finer-grained options:
    delete=False # don't delete
    delete=True # delete after file closed, current behavior
    delete=AFTER_CLOSE # delete after file closed
    delete=AFTER_CM_EXIT # delete after context manager exits
    delete=AFTER_CM_EXIT_NO_EXCEPTION # delete after CM exit, unless this is due to an exception

    I have implemented a Windows-friendly solution to the latter case using Nick Coghlan's code. My version does not delete the file until the context manager exits, and *if* the context manager exits due to an exception it leaves the file in place and reports its location, to aid me in debugging.

    @sorcio
    Copy link
    Mannequin

    sorcio mannequin commented Jun 30, 2012

    Daniel, Nick, shouldn't the context manager yield f within a with block?

    @sbt
    Copy link
    Mannequin

    sbt mannequin commented Jun 30, 2012

    Rather than add a NamedTemporaryFile.delete_after() classmethod, would it not be simpler to just add a close_without_unlink() method to NamedTemporaryFile?

        with NamedTemporaryFile() as f:
            <write to f>
            f.close_without_unlink()
            with open(f.name, 'rb') as f:
                <read from f>

    @dlenski
    Copy link
    Mannequin

    dlenski mannequin commented Jun 30, 2012

    Davide, the @contextlib.contextmanager decorator effectively wraps the
    yield statement in the necessary glue so that everything prior to the yield
    statement occurs in the __enter__() method of the contextmanager, while
    everything subsequent occurs in the __exit__() method.

    On Sat, Jun 30, 2012 at 1:46 AM, Davide Rizzo <report@bugs.python.org>wrote:

    Davide Rizzo <sorcio@gmail.com> added the comment:

    Daniel, Nick, shouldn't the context manager yield f within a with block?

    ----------
    nosy: +davide.rizzo


    Python tracker <report@bugs.python.org>
    <http://bugs.python.org/issue14243\>


    @dlenski dlenski mannequin changed the title tempfile.NamedTemporaryFile not particularly useful on Windows tempfile.NamedTemporaryFile not particularly useful on Windows Jun 30, 2012
    @jaraco
    Copy link
    Member

    jaraco commented Apr 11, 2021

    At least I and Ethan and Martin have expressed a desire for the default, preferred usage work well in a portable environment. Requiring delete_on_close=False violates that expectation.

    How about something like this instead:

    • Add an delete_when=None, also accepting "close" and "exit".
    • "close" means delete on close.
    • "exit" means delete when the context manager exits.
    • When delete_when is None, the default behavior is selected (currently close).
    • At some point (now or in the future), raise a deprecation warning if delete_when=None is passed (require it to be explicit) and explain that the default in the future will be delete_when="exit".
    • Document that passing an explicit None for delete_when is not supported (don't do it).
    • In a release after the deprecation has been released, change the default to delete_when="exit" and drop support for None.

    Note, this deprecation approach could be enacted with "delete_on_close" and boolean values, but I prefer more explicit values for shorter-named variables.

    This approach would allow a user to opt in to the future behavior which has the desired effect of preferring the default behavior (in as little as two releases).

    I might be tempted to create a backports package for users of earlier Python versions to get the future behavior sooner.

    @ethanfurman
    Copy link
    Member

    ethanfurman commented Apr 12, 2021

    On 4/11/2021 3:51 PM, Jason R. Coombs wrote:

    Jason R. Coombs jaraco@jaraco.com added the comment:

    At least I and Ethan and Martin have expressed a desire for the
    default, preferred usage work well in a portable environment. Requiring
    delete_on_close=False violates that expectation.

    My opinion is that no extra flags are necessary. The default of
    deleting on close is fine, unless a context manager is active -- in
    which case delete on CM exit. Note that an internal flag will be needed
    to track the status of being in a context manager, but nothing besides
    behavior in that edge case would change.

    @eryksun
    Copy link
    Contributor

    eryksun commented Apr 12, 2021

    My opinion is that no extra flags are necessary. The default of
    deleting on close is fine, unless a context manager is active -- in
    which case delete on CM exit.

    There is a use case of needing to let another thread or process open the temporary file while in a given context, but ensure that the file is deleted when the context exits. The O_TEMPORARY flag is not generally compatible with this use case, since very few programs in Windows share delete access. Python's open() doesn't, not without an opener. So this case needs to omit the O_TEMPORARY flag and rely on the context manager to delete the file.

    If there's no need to reopen the file in another thread or process, then using O_TEMPORARY is preferred. In this case, the file will deleted even if the current process crashes or gets terminated (e.g. by a job object).

    NamedTemporaryFile() in Windows could switch to relying on the context manager to delete the file. But also add an implementation of TemporaryFile() in Windows that uses O_TEMPORARY. Surely if a script has no need to reopen a temporary file, then it shouldn't care what the file's name is.

    For example:

        def TemporaryFile(mode='w+b', buffering=-1, encoding=None,
                          newline=None, suffix=None, prefix=None,
                          dir=None, *, errors=None):
            if "b" not in mode:
                encoding = _io.text_encoding(encoding)
        prefix, suffix, dir, output_type = _sanitize_params(prefix, suffix, dir)
        flags = _bin_openflags | _os.O_TEMPORARY
    
        (fd, name) = _mkstemp_inner(dir, prefix, suffix, flags, output_type)
        try:
            _os.unlink(name)
            return _io.open(fd, mode, buffering=buffering,
                            newline=newline, encoding=encoding, errors=errors)
        except:
            _os.close(fd)
            raise
    

    Prior to Windows 10 (tested back to Python 3.2 in Windows 2000), the unlink(name) call will leave the file linked in dir, but trying to access it with a new open will fail with an access-denied error. A file that's in a deleted state is only accessible by existing opens. The downside is that the temporary file can't be moved to another directory except by an existing open (e.g. via SetFileInformationByHandle: FileRenameInfo). Another process that wants to delete dir won't be able to move the file out of the way. It shouldn't be an issue, however, if the file is created in the user's temp directory.

    In Windows 10, NTFS implements a POSIX delete that also moves the file into a reserved system directory, so it doesn't remain linked in dir and thus doesn't prevent deleting dir.

    @ethanfurman
    Copy link
    Member

    ethanfurman commented Apr 12, 2021

    Eryk, I'm not sure if you are agreeing or disagreeing with me. :)

    On Windows it sounds like O_TEMPORARY buys us guaranteed file deletion, but costs us easy sharing of file resources and a difference in semantics between Windows and non-Windows.

    Is the automatic deletion truly that valuable?

    @zooba
    Copy link
    Member

    zooba commented Apr 12, 2021

    O_TEMPORARY is clearly not the right option here, and we should just move the unlink call into __exit__ and attempt it even if close() has been called.

    Windows's "delete on close" functionality is great, but if you haven't designed for its semantics, it's unusable. And this library is not designed for it, so users can't rely on the native functionality.

    So we stop passing the O_TEMPORARY flag. If __enter__() is called, close() closes the file but doesn't delete anything, and __exit__() closes the file (if open) and deletes it (even if it wasn't open). If there is no __enter__(), close() also deletes the file.

    @pfmoore
    Copy link
    Member

    pfmoore commented Apr 12, 2021

    There's a lot of technical discussion of implementation details here, but not much about use cases. IMO, what's more important is whether NamedTemporaryFile is *useful* to people, and what they want to use it *for*. Working out how to implement it can come after we know what people want it to do.

    My particular use case is actually pretty simple, and I suspect constitutes a fairly major proportion of what people want of this API:

    1. Create a temporary file.
    2. Write some data into it.
    3. At some point, we're done writing to the file.
    4. Only after that point, pass the name of the file to "somewhere else" for processing. That's often, in my use cases, as an argument to a subprocess.
    5. Once we're all done, clean up securely.

    The key additional requirement is that this is done "safely" (by which I mean I don't have to think about race conditions, etc, as someone else has that covered). What I think that means is that we need to maintain an open filehandle of *some* sort continually, but there's a point where the user can say "I'm done writing, I want to share this now".

    Is there an actual known use case for the behaviour of deleting the file on close *rather* than at the end of the CM's scope? That's unlike any other CM I know of, where the scope ending is what triggers tidy-up.

    Maybe NamedTemporaryFile should be retained for backward compatibility, but as a normal function, with the CM behaviour deprecated. It would still be essentially useless on Windows, but we maybe don't care about that.

    In addition, we create a *new* context manager, that simply creates the file at the start and deletes it at close of scope. It returns a writeable file object, but that can be closed (and the documentation notes that for portability it *must* be closed before passing the name to another process for use).

    I don't know enough about how we protect this against race condition attacks, but I'm pretty sure that this API gives us the best chance we're likely to have of doing so in a cross-platform manner. (Maybe we have a "reopen" method rather than "close". Or can we open *two* file handles at the start, one for writing and one for reading, return the one for writing, and keep the one for reading internal, purely to keep the file locked? I feel like that wouldn't work because if *we* can open a write handle, so could an attacker - but as I say, I'm not an expert).

    Basically, I may be wrong, but I feel that we should stop trying to "rescue" NamedTemporaryFile, and instead try providing an *alternative* that handles the cross-platform use case that started all of this.

    @ethanfurman
    Copy link
    Member

    ethanfurman commented Apr 12, 2021

    Paul,

    If "rescuing" (i.e. "fixing" ;) NamedTemporaryFile was arduous, complicated, or had serious backwards-compatibility issues then I would completely agree with you. However, the fix is simple, the only backwards-compatible issue is the file would still be there /while a context manager was in use/ after it had been closed (which conforms to most, if not all, users of context managers) and a file would be left on disk in the event of a hard crash (hopefully a rare occurrence).

    Your proposal, on the other, is a lot of work. Is the minor backwards compatibility worth all that work?

    @pfmoore
    Copy link
    Member

    pfmoore commented Apr 12, 2021

    Sorry - I'm maybe making an unwarranted assumption. If simply removing "delete on close" behaviour in the CM case is acceptable, then I'm 100% in favour of that.

    I'd assumed that it was somehow unacceptable, but you're right, and it's not clear if Eryk is agreeing or disagreeing with you (I assumed he was disagreeing, based mainly on the length and complexity of his response :-)) It didn't help that somehow Steve's reply wasn't visible when I posted mine.

    Apologies for the noise.

    @ethanfurman
    Copy link
    Member

    ethanfurman commented Apr 12, 2021

    Hey, you agree with me now, so it's not noise. ;-)

    @eryksun
    Copy link
    Contributor

    eryksun commented Apr 12, 2021

    So we stop passing the O_TEMPORARY flag. If __enter__() is called,
    close() closes the file but doesn't delete anything, and
    __exit__() closes the file (if open) and deletes it (even if it
    wasn't open). If there is no __enter__(), close() also deletes the
    file.

    This behavior change is fine if O_TEMPORARY isn't used. I wasn't disagreeing with Ethan. I was providing a summary of a common use case that conflicts with using O_TEMPORARY to make it clear that this flag has to be omitted if we're not implementing something like a delete_on_close boolean option.

    Most of my last comment, however, was dedicated to implementing TemporaryFile() if this change is applied, instead of leaving it as an alias for NamedTemporaryFile(). I can't imagine not wanting the guaranteed cleanup semantics of O_TEMPORARY in the case of an anonymous temporary file that doesn't need to be reopened. I also want O_SHORT_LIVED. This opens the file with the attribute FILE_ATTRIBUTE_TEMPORARY [1], which tells the cache manager to try to keep the file contents in memory instead of flushing data to disk.

    ---

    [1] https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew#caching_behavior

    @Ev2geny
    Copy link
    Mannequin

    Ev2geny mannequin commented Apr 28, 2021

    Dear all, thank you very much for the discussion, I will just try to summarize the results of it.

    In my PR I used solution, proposed by Eryk. My solution involves introduction of the extra flag delete_on_close and making sure, that new situation is fully backwards compatible, because I did not feel I have an authority to propose anything which would break backwards compatibility (as Python 4.0 I think is not on the horizon yet)

    As I see from the discussion, the following decisions need to be taken:

    WHICH FLAG TO USE

    Eryk was proposing that it shall be delete_on_close (which I have implemented)

    Jason thinks, that we shall introduce flag delete_when, however when I read his proposal, the functionality which he proposes is not that different from what I implemented with delete_on_close.

    Ethan however thinks, that no extra flags are necessary at all

    USAGE OF O_TEMPORARY ON WINDOWS

    Ethan, Steve thinks, it is not needed

    Eryk prefers to provide a way to omit O_TEMPORARY, but still use it by default, when it's omitted

    CHANGING OF THE CURRENT BEHAVIOUR / BREAKING BACKWARDS COMPATIBILITY
    Ethan thinks, that we shall slightly change backwards compatibility in a way that if CM is used, than file is deleted on CM exit and not on close as now

    Jason thinks that backwards compatibility shall be changed gradually over several releases with the advanced depreciation warning

    Question: does everybody definitely agree then, that backwards compatibility shall definitely be altered, whether immediately or gradually?

    Any other decision to be taken which I missed?

    CONCLUSION:
    There seems to be still disagreements and I don't really know how to move this forward as I am not sure I understand the decision making mechanism in the Python community (especially when it comes to breaking backwards compatibility). Any thoughts on this?

    @Ev2geny
    Copy link
    Mannequin

    Ev2geny mannequin commented Apr 28, 2021

    On Mon, Apr 12, 2021 at 12:51 AM Jason R. Coombs <report@bugs.python.org> wrote:

    Jason R. Coombs <jaraco@jaraco.com> added the comment:

    At least I and Ethan and Martin have expressed a desire for the default, preferred usage work well in a portable environment. Requiring delete_on_close=False violates that expectation.

    Separately to my previous message. If the only issue with my PR is that default solution is not portable, then this can be simply changed within current PR by setting default of delete_on_close to fe equal to False.

    As I mentioned, I was assuming I cannot break backwards compatibility.

    @pfmoore
    Copy link
    Member

    pfmoore commented Apr 28, 2021

    To be explicit, I'm +1 on breaking backward compatibility in the minor form described by Ethan: if NamedTemporaryFile is used as a context manager, the file is closed *on context manager exit* and *not* when the file is closed.

    Breaking compatibility is allowed in minor versions (3.11 at this point, as this won't make it in before 3.10 feature freeze). So this is an acceptable change.

    I don't personally think this needs a transition period or deprecation, and in particular I don't think that Jason's "gradual transition" proposal is necessary.

    I'd be sad if we ended up with an over-complicated solution for no better reason than an excess of caution over a backward compatibility issue that we're not sure will impact anyone. Do we have any actual examples of code that needs the current CM behaviour (as opposed to a general concern that someone might be using the functionality)?

    (My original over-complicated proposal was based on a mistaken belief that it had already been established that backward incompatibility was absolutely not allowed. But I never subscribed to that view myself.)

    @zware
    Copy link
    Member

    zware commented Apr 30, 2021

    if NamedTemporaryFile is used as a context manager, the file is closed *on context manager exit* and *not* when the file is closed.

    +1

    @pfmoore
    Copy link
    Member

    pfmoore commented Apr 30, 2021

    Looking at the various comments, I think we have 5 votes for deleting on CM exit when used as a CM, and no change in behaviour otherwise (me, Zachary, Ethan, Jason and Steve). Steve also wants O_TEMPORARY to be removed, which doesn't seem controversial among this group of people.

    Eryk has argued for a delete_on_close flag that would need to be explicitly set to False, retaining the use of O_TEMPORARY in the default case, but there doesn't seem to be a lot of support for that.

    If I've misrepresented anyone's view, please speak up!

    I didn't look back at the stuff from 2013 and earlier, I'll admit.

    I do think this needs care to implement (and document!) correctly. For example, consider the following case:

        ntf = NamedTemporaryFile()
        # Do some stuff (1)
        with ntf:
            # Do some stuff (2)
        # Do some followup stuff

    I assume we'd want a close in (1) to delete the file, but a close in (2) to leave it open until the CM exit.

    Evgeny, would you be willing to update your PR (including adding the docs change, and tests to catch as many edge cases as you can think up) to match this behaviour?

    @eryksun
    Copy link
    Contributor

    eryksun commented Apr 30, 2021

    deleting on CM exit when used as a CM, and no change in behaviour
    otherwise (me, Zachary, Ethan, Jason and Steve). Steve also wants
    O_TEMPORARY to be removed, which doesn't seem controversial among
    this group of people.

    Removing O_TEMPORARY is not an afterthought here. It is the core of this issue. The O_TEMPORARY flag MUST NOT be used if the goal is to make NamedTemporaryFile() "particularly useful on Windows". A file that's opened with DELETE access cannot be reopened in most cases, because most opens do not share delete access, but it also can't be closed to allow it to be reopened because the OS will delete it.

    I replied twice that I thought using the CM exit instead of O_TEMPORARY is okay for NamedTemporaryFile() -- but only if a separate implementation of TemporaryFile() that uses O_TEMPORARY is added at the same time. I want guaranteed cleanup for TemporaryFile() since it's not intended to be reopened.

    @Ev2geny
    Copy link
    Mannequin

    Ev2geny mannequin commented Apr 30, 2021

    Eryk Sun <eryksun@gmail.com> added the comment:

    I replied twice that I thought using the CM exit instead of O_TEMPORARY is okay for NamedTemporaryFile() -- but only if a separate implementation of TemporaryFile() that uses O_TEMPORARY is added at the same time. I want guaranteed cleanup for TemporaryFile() since it's not intended to be reopened.

    At the moment, the TemporaryFile directly reuses NamedTemporaryFile for none-posix or cygwin systems.

    if _os.name != 'posix' or _sys.platform == 'cygwin':

    Does it mean, that your suggestion to leave the O_TEMPORARY for TemporaryFile means, that NamedTemporaryFile needs to have a mechanism to know whether it was called as a TemporaryFile and then to have a different functionality in this case relative to the situation it would be called directly?

    @Ev2geny
    Copy link
    Mannequin

    Ev2geny mannequin commented Apr 30, 2021

    Paul Moore <p.f.moore@gmail.com> added the comment:

    Evgeny, would you be willing to update your PR (including adding the docs change, and tests to catch as many edge cases as you can think up) to match this behaviour?

    Paul, thank you for moving this forward. I will give it a try.

    @eryksun
    Copy link
    Contributor

    eryksun commented Apr 30, 2021

    Does it mean, that your suggestion to leave the O_TEMPORARY for
    TemporaryFile means, that NamedTemporaryFile needs to have a
    mechanism to know whether it was called as a TemporaryFile

    Just implement a separate function for TemporaryFile() instead of aliasing it to NamedTemporaryFile(). See msg390814.

    @pfmoore
    Copy link
    Member

    pfmoore commented Apr 30, 2021

    Eryk, thank you for clarifying. I apologise - I got bogged down somewhere in the middle of the discussion on reimplementing bits of the CRT (your posts are so information-dense that my usual habit of skimming breaks down - that's not a complaint, though!)

    @Ev2geny
    Copy link
    Mannequin

    Ev2geny mannequin commented Apr 30, 2021

    Eryk Sun <eryksun@gmail.com> added the comment:
    Just implement a separate function for TemporaryFile() instead of aliasing it to NamedTemporaryFile(). See msg390814.

    Eryk, forgive my ignorance, but aren't in your msg390814 you are proposing yet another enhancement (separate from the bpo-14243, discussed here), in this case for TemporaryFile in Windows systems?

    I may be mistaken, but I see that you are proposing some trick with first unlinking the file and then employing it as a temporary file using previously known fd. This "trick" is not present in the current code and does not seem to address bpo-14243.

    Or am I talking total nonsense?

    @eryksun
    Copy link
    Contributor

    eryksun commented Apr 30, 2021

    you are proposing some trick with first unlinking the file and
    then employing it as a temporary file using previously known fd.

    It doesn't need to unlink the file to make it anonymous. I included that to highlight that it's possible. The details can be discussed and hashed out in the PR.

    I don't think implementing TemporaryFile() in Windows is separate from this issue. It goes hand in hand with the decision to stop using O_TEMPORARY in NamedTemporaryFile().

    @Ev2geny
    Copy link
    Mannequin

    Ev2geny mannequin commented Apr 30, 2021

    Eryk, I agree, that implementing TemporaryFile() in Windows goes hand in hand with the decision to stop using O_TEMPORARY in NamedTemporaryFile()

    The only thing I want to point out is that your suggestion also includes this "unlinking trick" (sorry, may be there is a better description for this), which seems to be separate/extra to the usage of O_TEMPORARY.

    Hence my newbie questions are:

    1. What problem are you trying to solve by this "unlinking trick"?

    2. Do we need to have a separate issue raised for this problem?

    3. Is this "unlinking trick" using some documented features of Windows? (will it not stop working after some Windows patch)?

    4. Will we need to create separate unit tests for this issue?

    5. Do we also need to reach a consensus on implementing of this "unlinking trick"?

    @methane
    Copy link
    Member

    methane commented May 1, 2021

    +1 to Eryk.

    Hence my newbie questions are:

    1. What problem are you trying to solve by this "unlinking trick"?

    Same to TempoaryFile in Unix.

    1. Do we need to have a separate issue raised for this problem?

    I don't think so. We didn't unlink because wi didn't have separate implementation for TemporaryFile and NamedTemporaryFile.
    When we have two implementations for them, it is straightforward and natural to use unlink trick.

    1. Is this "unlinking trick" using some documented features of Windows? (will it not stop working after some Windows patch)?

    I am not sure. But it is "POSIX function" in windows. I believe MS won't break compatibility.

    1. Will we need to create separate unit tests for this issue?

    We already have tests for TemporaryFile.

    1. Do we also need to reach a consensus on implementing of this "unlinking trick"?

    If anyone against it. But I think Eryk's proposal is the most reasonable.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.10 OS-windows stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    Status: No status
    Development

    No branches or pull requests