Skip to content

Built-in module _io can lose data from buffered files in reference cycles #62052

Closed

Description

@arigo
mannequin
BPO 17852
Nosy @tim-one, @nascheme, @pitrou, @vstinner, @njsmith, @methane, @vadmium, @serhiy-storchaka, @jstasiak, @xgid, @nitishch, @Arusekk, @miss-islington, @Volker-Weissmann
PRs
  • bpo-17852: Maintain a list of BufferedWriter objects. Flush them on exit. #1908
  • Revert "bpo-17852: Maintain a list of BufferedWriter objects. Flush them on exit." #3337
  • bpo-17852: Maintain a list of BufferedWriter objects. Flush them on exit. #3372
  • bpo-17852: Maintain a list of BufferedWriter objects. Flush them on exit. #3372
  • bpo-17852: Revert incorrect fix based on misunderstanding of _Py_PyAtExit() semantics #4826
  • bpo-17852: Fix PR #3372, flush BufferedWriter objects on exit. #4847
  • bpo-17852: Fixed the documentation about closing files #23135
  • [3.9] bpo-17852: Doc: Fix the tutorial about closing files (GH-23135) #23527
  • [3.8] bpo-17852: Doc: Fix the tutorial about closing files (GH-23135) #23528
  • Files
  • y.py
  • gcio.py
  • pyio.diff
  • buffer_crash.py
  • buffer_not_flushed.py
  • buffer_register_flush.txt
  • 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 = 'https://github.com/nascheme'
    closed_at = None
    created_at = <Date 2013-04-27.07:33:30.917>
    labels = ['type-bug', '3.7', 'expert-IO']
    title = 'Built-in module _io can lose data from buffered files in reference cycles'
    updated_at = <Date 2020-12-01.10:53:50.492>
    user = 'https://github.com/arigo'

    bugs.python.org fields:

    activity = <Date 2020-12-01.10:53:50.492>
    actor = 'miss-islington'
    assignee = 'nascheme'
    closed = False
    closed_date = None
    closer = None
    components = ['IO']
    creation = <Date 2013-04-27.07:33:30.917>
    creator = 'arigo'
    dependencies = []
    files = ['30030', '37357', '37359', '47121', '47332', '47340']
    hgrepos = []
    issue_num = 17852
    keywords = ['patch']
    message_count = 71.0
    messages = ['187890', '187898', '187903', '187922', '187966', '188075', '188077', '188095', '188130', '188137', '231376', '231386', '231391', '231420', '232103', '232109', '232113', '232114', '232116', '232117', '232118', '232120', '232121', '232122', '232123', '232124', '232126', '232127', '232129', '232131', '232132', '232133', '232134', '232137', '232139', '232140', '232146', '294868', '294910', '294956', '301295', '301297', '301329', '301331', '301353', '301361', '301363', '302758', '305653', '305665', '308170', '308177', '308236', '308330', '308332', '308412', '308417', '308419', '308424', '308666', '308668', '308681', '308787', '310602', '310648', '310659', '355064', '381924', '381925', '381926', '382232']
    nosy_count = 14.0
    nosy_names = ['tim.peters', 'nascheme', 'pitrou', 'vstinner', 'njs', 'methane', 'martin.panter', 'serhiy.storchaka', 'jstasiak', 'xgdomingo', 'nitishch', 'Arusekk', 'miss-islington', 'Volker Wei\xc3\x9fmann']
    pr_nums = ['1908', '3337', '3372', '3372', '4826', '4847', '23135', '23527', '23528']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue17852'
    versions = ['Python 3.6', 'Python 3.7']

    Linked PRs

    Activity

    1. arigo commented on Nov 20, 2014

      arigomannequin
      MannequinAuthor

      Victor: there is the GIL, you don't need any locking.

    2. nikratio commented on Dec 4, 2014

      nikratiomannequin
      Mannequin

      Serhiy, I believe this still happens in Python 3.4, but it is harder to reproduce. I couldn't get Armin's script to produce the problem either, but I'm pretty sure that this is what causes e.g. https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=771452#60.

    3. neologix commented on Dec 4, 2014

      neologixmannequin
      Mannequin

      Serhiy, I believe this still happens in Python 3.4, but it is harder to reproduce. I couldn't get Armin's script to produce the problem either, but I'm pretty sure that this is what causes e.g. https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=771452#60.

      Maybe, as it's been said several times, it's an application bug: files
      should be closed explicitly (or better with a context manager of
      course).

    4. arigo commented on Dec 4, 2014

      arigomannequin
      MannequinAuthor

      I hate to repeat myself, but if the C standard says "files are flushed at exit"; if Python 2 follows this standard; and if Python 3 follows it most of the time but *not always*... then it seems to me that something is very, very buggy in the worst possible way (rare data loss).

    5. serhiy-storchaka commented on Dec 4, 2014

      @serhiy-storchaka
      Member

      Do you have working in 3.4+ example?

    6. arigo commented on Dec 4, 2014

      arigomannequin
      MannequinAuthor

      If I understood correctly, Python 3.4 tries harder to find cycles and call destructors at the end of the program, but that's not a full guarantee. For example you can have a reference from a random C extension module.

      While trying to come up with an example, I found one that I don't fully understand, but the point is that it shows how easy it is to defeats it:

          import sys
          f = open('foo.txt', 'w')
          f.write('abc')
          def func(*args):
              return func
          sys.settrace(func)
    7. arigo commented on Dec 4, 2014

      arigomannequin
      MannequinAuthor

      (Ah, it's probably a reference from the trace function -> func_globals -> f).

    8. pitrou commented on Dec 4, 2014

      @pitrou
      Member

      Here is an example.

    9. pitrou commented on Dec 4, 2014

      @pitrou
      Member

      Note the example I posted doesn't involve the shutdown sequence. It calls gc.collect() explicitly.

    10. arigo commented on Dec 4, 2014

      arigomannequin
      MannequinAuthor

      To add to the confusion: Antoine's example produces an empty file on the current trunk cd282dd0cfe8. When I first tried it on a slightly older trunk (157 changesets ago), it correctly emitted a file with "barXXX ", but only if "gc.collect()" was present. Without "gc.collect()" it didn't. This seems at odds with the explanation that "gc.collect()" should be what occurs at shut-down now.

    11. pitrou commented on Dec 4, 2014

      @pitrou
      Member

      The problem is that the order of tp_finalize calls is arbitrary when there is a reference cycle (same thing, of course, with tp_clear). So depending on the exact layout of the garbage list, the TextIOWrapper could be collected before the BufferedWriter and the FileIO (good), or after (bad).

      I don't see an easy way to solve this. Either _io should provide hints to the GC (which kind of hints?), or the tp_finalize should be customized to somehow call a dependent wrapper's tp_finalize (how? by keeping a weakref?).

    12. arigo commented on Dec 4, 2014

      arigomannequin
      MannequinAuthor

      Maybe accepting the fact that relying on finalizers is a bad idea here?

    13. pitrou commented on Dec 4, 2014

      @pitrou
      Member

      You mean encourage people to explicitly use "with" or call close()? Yes, that's the simplest answer. It's why we added ResourceWarning.

    14. vstinner commented on Dec 4, 2014

      @vstinner
      Member

      The problem is that the order of tp_finalize calls is arbitrary when there is a reference cycle

      If we want to guaranty that all files are properly flushed at exit, the best option is to maintain a list of open files.

      I'm not interested to implement that, it sounds too complex. For example, open("text.txt", "w") creates 3 objects: FileIO, BufferedWriter, TextIOWrapper. You have to register these 3 objects and flush them in the right order. Python is dynamic, you are free to modify objects, for example using the detach() method or by modifying the buffer attribute of the TextIOWrapper.

      How do you decide which object should be flushed? In which order? We have to take care of signals, threads and forks, stay portable, etc.

    15. pitrou commented on Dec 4, 2014

      @pitrou
      Member

      Le 04/12/2014 12:05, STINNER Victor a écrit :

      > The problem is that the order of tp_finalize calls is arbitrary
      > when
      there is a reference cycle

      If we want to guaranty that all files are properly flushed at exit,
      the best option is to maintain a list of open files.

      I am not talking about files flushing at exit.

    16. serhiy-storchaka commented on Dec 4, 2014

      @serhiy-storchaka
      Member

      How do you decide which object should be flushed? In which order? We have to
      take care of signals, threads and forks, stay portable, etc.

      The order doesn't matter. If you call flush() of TextIOWrapper, flushes of
      buffered writer and raw file will be called automatically. And flush() can be
      called multiple times.

    17. arigo commented on Dec 4, 2014

      arigomannequin
      MannequinAuthor

      Antoine: sorry if I wasn't clear enough. Obviously you want to encourage people to close their files, but I think personally that it is very bad for the implementation to *most of the time* work anyway and only rarely fail to flush the files. So, speaking only about the implementation, it is (imho) a bad idea to rely on finalizers to flush the files, and something else should be done.

      Victor: it does not sound complicated to me to keep the BufferedWriter objects in a doubly-chained list. You're overthinking the issue: there are no multithread issue (we have a GIL) and you don't have to keep track of all 3 objects created by "open".

    18. pitrou commented on Dec 4, 2014

      @pitrou
      Member

      Le 04/12/2014 13:53, Armin Rigo a écrit :

      So, speaking only about the
      implementation, it is (imho) a bad idea to rely on finalizers to flush
      the files, and something else should be done.

      But what else?

    19. arigo commented on Dec 4, 2014

      arigomannequin
      MannequinAuthor

      Antoine: I'm trying to explain what in the three lines that follow the parts you quoted. I already tried to explain it a few times above. Now I feel that I'm not going anywhere, so I'll quote back myself from 2013-04-27: "Feel free to close anyway as not-a-bug; I won't fight the Python 3 behavior, because Python 2 works as expected."

    20. pitrou commented on Dec 4, 2014

      @pitrou
      Member

      Armin: I don't see how linked lists would help.

    21. serhiy-storchaka commented on Dec 4, 2014

      @serhiy-storchaka
      Member

      How it looks to me. Should be global collection of weak references (WeakSet?).
      Every instance of TextIOWrapper, BufferedWriter and BufferedRandom add itself to
      this collection on create and remove on close. A function registered with
      atexit calls flush() for all methods in the collection.

      This solution doesn't require changes to finalization machinery, only to the io
      module.

      I'm not particularly interested to implement that because I'm not convinced
      that Python should provide such guaranties. But the implementation shouldn't
      be too complex.

      More general solution could be to introduce special method (predel) which
      should be called before breaking reference loops (or just before __del__ if
      there is no loop). In case of output streams it will call flush() (but not
      close()!).

    22. arigo commented on Dec 4, 2014

      arigomannequin
      MannequinAuthor

      Here is a proof-of-concept. It changes both _pyio.py and bufferedio.c and was tested with the examples here. (See what I meant with "linked lists".) The change in textio.c is still missing, but should be very similar to bufferedio.c. This is similar to the implementation in PyPy.

    23. vstinner commented on Dec 4, 2014

      @vstinner
      Member

      By the way, I fixed various issues to ensure that ResourceWarning are
      displayed at Python exit. There are still corner cases when the
      warnings are not emited. For example, when a daemon thread is used. Or
      when the warning comes very late during Python finalization.

      Recent example of tricky issue related to this warning: bpo-22898. IMO
      it's a bug, it must be fixed and it should be possible to emit the
      warning.

    24. nikratio commented on Dec 4, 2014

      nikratiomannequin
      Mannequin

      This will probably be too radial, but I think it should at least be mentioned as a possible option.

      We could just not attempt to implicitly flush buffers in the finalizer at all. This means scripts relying on this will break, but in contrast to the current behavior they will break consistently so it's easy to find and fix the problem.

      I think this is still preferable to any solution that does not guarantee flushing in 100% of the cases.

    25. nascheme commented on May 31, 2017

      @nascheme
      Member

      Well, I just spent a couple of hours debugging a problem caused by this issue. You could argue that I should be calling close() on all of my file-like objects but I agree with Armin that the current "most of the time it works" behaviour is quite poor.

      In my case, the issue is exactly what Antoine Pitrou suggests: if the FileIO object gets finalized before the BufferedIO object wrapping it then the buffered data gets lost. That depends on the order that the GC calls finalizers.

      Armin's suggestion of keeping a list of open buffered files and flushing them before exiting seems like a simple and robust solution. Having a warning for unclosed files is fine but in order to make porting Python 2 code as painless as possible, matching the safer behavior of Python 2 would be worth the extra bit of flush logic.

    26. vstinner commented on Jun 1, 2017

      @vstinner
      Member

      Neil Schemenauer: "Well, I just spent a couple of hours debugging a problem caused by this issue."

      Did you get any ResourceWarning? You need to run python3 with -Wd to see them.

      By the way, I enhanced ResourceWarning in Python 3.6: if you enable tracemalloc, these warnings now log the traceback where the leakded resource was created ;-)
      https://docs.python.org/dev/whatsnew/3.6.html#warnings

    27. nascheme commented on Jun 1, 2017

      @nascheme
      Member

      "Did you get any ResourceWarning?"

      I already knew that explicitly closing the file would fix the issue. However, think of the millions of lines of Python 2 that hopefully will be converted to Python 3. There will be many ResourceWarning errors. It is not reasonable to think that everyone is just going to fix them when they see them. Applications that used to be reliable will now randomly lose data.

      What I spent hours debugging is trying to figure out why when the open file is collected on interpreter exit, buffered data not flushed. I knew the object was not part of an uncollectable garbage cycle. I initially suspected there could be some bug in the _io module or maybe even a kernel bug.

      It turns out that in Python 2 the buffer and the open file is a single object and so when the finalizer gets called, the data is always correctly written out. In Python 3, the buffer and the underlying file object are separate and they can get finalized in different order depending on essentially random factors.

      Fixing this particular issue is not difficult as Armin's code demonstrates. I don't think blaming the application programmer is the solution. Obviously we keep the warnings and still tell programmers to close their files or use context managers.

    28. nascheme commented on Sep 5, 2017

      @nascheme
      Member

      New changeset e38d12e by Neil Schemenauer in branch 'master':
      bpo-17852: Maintain a list of BufferedWriter objects. Flush them on exit. (bpo-1908)
      e38d12e

    29. nascheme commented on Sep 5, 2017

      @nascheme
      Member

      New changeset db56423 by Neil Schemenauer in branch 'master':
      Revert "bpo-17852: Maintain a list of BufferedWriter objects. Flush them on exit. (bpo-1908)" (bpo-3337)
      db56423

    30. vstinner commented on Sep 5, 2017

      @vstinner
      Member

      New changeset e38d12e by Neil Schemenauer in branch 'master':
      bpo-17852: Maintain a list of BufferedWriter objects. Flush them on exit. (bpo-1908)

      This change introduced a regression:

      ======================================================================
      FAIL: test_4_daemon_threads (test.test_threading.ThreadJoinOnShutdown)
      ----------------------------------------------------------------------

      Traceback (most recent call last):
        File "/Users/db3l/buildarea/3.x.bolen-tiger/build/Lib/test/test_threading.py", line 781, in test_4_daemon_threads
          self.assertFalse(err)
      AssertionError: b'pthread_cond_destroy: Resource busy\npthread_mutex_destroy: Resource busy' is not false

      http://buildbot.python.org/all/builders/x86%20Tiger%203.x/builds/1137

      New changeset db56423 by Neil Schemenauer in branch 'master':
      Revert "bpo-17852: Maintain a list of BufferedWriter objects. Flush them on exit. (bpo-1908)" (bpo-3337)

      This revert fixed the regression.

      Did you revert the change because of the failure?

    31. nascheme commented on Sep 5, 2017

      @nascheme
      Member

      I reverted because of the crash in test_threading. I'm pretty sure there is a bug with the locking of bufferedio.c, related to threads and flush. Here is the stacktrace I get (my patch applied, I'm trying to write a Python test that triggers the SEGV without my flush patch).

      Attached is the script that triggers the crash.

      Thread 3 received signal SIGSEGV, Segmentation fault.
      0x00000001000c06d4 in PyObject_GetAttr (
      v=<unknown at remote 0xdbdbdbdbdbdbdbdb>, name='seek')
      at ../Objects/object.c:882
      882 PyTypeObject *tp = Py_TYPE(v);
      (gdb) bt
      #0 0x00000001000c06d4 in PyObject_GetAttr (
      v=<unknown at remote 0xdbdbdbdbdbdbdbdb>, name='seek')
      at ../Objects/object.c:882
      #1 0x00000001000440e9 in PyObject_CallMethodObjArgs (
      callable=<unknown at remote 0xdbdbdbdbdbdbdbdb>, name='seek')
      at ../Objects/call.c:1212
      #2 0x00000001003005a2 in _buffered_raw_seek (self=0x1053a39b8, target=0,
      whence=1) at ../Modules/_io/bufferedio.c:742
      #3 0x000000010030015e in buffered_flush_and_rewind_unlocked (self=0x1053a39b8)
      at ../Modules/_io/bufferedio.c:851
      #4 0x00000001002fed12 in buffered_flush (self=0x1053a39b8, args=0x0)
      at ../Modules/_io/bufferedio.c:869
      #5 0x00000001002feb4d in _PyIO_atexit_flush ()
      at ../Modules/_io/bufferedio.c:1863
      [...]

    32. pitrou commented on Sep 5, 2017

      @pitrou
      Member

      Just apply the following patch to the original PR and it should work fine:

      diff --git a/Modules/_io/bufferedio.c b/Modules/_io/bufferedio.c
      index 50c87c1..2ba98f2 100644
      --- a/Modules/_io/bufferedio.c
      +++ b/Modules/_io/bufferedio.c
      @@ -409,12 +409,12 @@ static void
       buffered_dealloc(buffered *self)
       {
           self->finalizing = 1;
      +    if (self->next != NULL)
      +        remove_from_linked_list(self);
           if (_PyIOBase_finalize((PyObject *) self) < 0)
               return;
           _PyObject_GC_UNTRACK(self);
           self->ok = 0;
      -    if (self->next != NULL)
      -        remove_from_linked_list(self);
           if (self->weakreflist != NULL)
               PyObject_ClearWeakRefs((PyObject *)self);
           Py_CLEAR(self->raw);
      @@ -1860,8 +1860,12 @@ void _PyIO_atexit_flush(void)
           while (buffer_list_end.next != &buffer_list_end) {
               buffered *buf = buffer_list_end.next;
               remove_from_linked_list(buf);
      -        buffered_flush(buf, NULL);
      -        PyErr_Clear();
      +        if (buf->ok && !buf->finalizing) {
      +            Py_INCREF(buf);
      +            buffered_flush(buf, NULL);
      +            Py_DECREF(buf);
      +            PyErr_Clear();
      +        }
           }
       }
    33. pitrou commented on Sep 5, 2017

      @pitrou
      Member

      To elaborate a bit on the patch:

      • it is pointless to call flush() if the buffered is in a bad state (self->ok == 0) or it has started finalizing already
      • you need to own the reference, since flush() can release the GIL and, if the reference is borrowed, the refcount can fall to 0 in another thread and the whole object deallocated under your feet
    34. changed the title [-]Built-in module _io can loose data from buffered files at exit[/-] [+]Built-in module _io can lose data from buffered files at exit[/+] on Sep 6, 2017
    35. nascheme commented on Sep 22, 2017

      @nascheme
      Member

      New changeset 0a1ff24 by Neil Schemenauer in branch 'master':
      bpo-17852: Maintain a list of BufferedWriter objects. Flush them on exit. (bpo-3372)
      0a1ff24

    36. vstinner commented on Nov 6, 2017

      @vstinner
      Member

      Neil pushed the commit 0a1ff24. What is the status of this issue? Can we now close it?

    37. nascheme commented on Nov 6, 2017

      @nascheme
      Member

      Yes, my bad. I thought that accepting the pull would close the bug.

    38. pitrou commented on Dec 12, 2017

      @pitrou
      Member

      I just discovered that the fix is incorrect. See PR bpo-4826 for reversion and a quick explanation.

    39. reopened this on Dec 12, 2017
    40. vstinner commented on Dec 13, 2017

      @vstinner
      Member

      New changeset 317def9 by Victor Stinner (Antoine Pitrou) in branch 'master':
      bpo-17852: Revert incorrect fix based on misunderstanding of _Py_PyAtExit() semantics (bpo-4826)
      317def9

    41. nascheme commented on Dec 13, 2017

      @nascheme
      Member

      I created a new PR which uses the atexit module instead of using _Py_PyAtExit. I think registering in io.py is okay.

      I see that atexit is now implemented in C. Rather than registering in io.py, we could create a C API to register callbacks (i.e. atexit_register). That would perhaps be a bit cleaner.

    42. nascheme commented on Dec 14, 2017

      @nascheme
      Member

      Attached is a script that triggers the non-flushing behaviour for me. I don't think it is reliable since it depends on the order that FileIO AND BufferedWriter are finalized when the gc finds them in a reference cycle.

      BTW, it is arguable that the root cause of this bug is that we no longer to topological ordering when calling finalizers. Originally, the cycle GC would refuse to call __del__ methods because once they are part of a cycle, there is no defined topological ordering. So, we linked that garage to gc.garbage rather than calling __del__ and have potentially undefined results. Now the GC has been changed to call __del__ anyhow. I haven't studied how this has been changed but this non-flushing bug is a result. Having the buffer added to gc.garbage would also result in the data not being flushed but arguably it would be more understandable what's going on. I'm not arguing that we should go back to that, just that current behaviour can be subtle and confusing.

    43. pitrou commented on Dec 14, 2017

      @pitrou
      Member

      The reason Python 2 did well here is simply that Python 2 had a single Python object (the file object) for each actual file. Python 3 has several of them (the raw IO object, the buffered IO object, possibly the text IO wrapper), and so suddenly the finalization order matters.

    44. nascheme commented on Dec 15, 2017

      @nascheme
      Member

      In the process of trying to write a test for this, I now realize that PR 4847 is not really a fix. If the underlying file gets closed during an earlier gc.collect() and not during shutdown, the extra flushing step is not going to help. So, using atexit is not really a fix. Maybe there is something else we can do, need to think about it more.

    45. serhiy-storchaka commented on Dec 15, 2017

      @serhiy-storchaka
      Member

      I'm not well known with GC, but if it be possible to call destructors in the reversed chronological order, this could help with this issue.

    46. nascheme commented on Dec 15, 2017

      @nascheme
      Member

      Using reversed chronological order would work in 99% of the cases probably but then you would have that rare case where it didn't work. So, I'm not too keen on that approach.

      I think this is a classic problem with finalization and GC, probably should do some actual reading of literature. One idea that occurs to me is that the underlying file object could keep a weakref to the higher level (e.g. buffer) objects. Then, if the underlying file gets closes, it could make sure to call flush on the higher level objects first. The weakref would ensure that a ref cycle is not created.

    47. nikratio commented on Dec 15, 2017

      nikratiomannequin
      Mannequin

      Given the apparent difficulties getting this right, how about not attempting to implicitly flush buffers in the finalizer at all? This means scripts relying on this will break, but in contrast to the current behavior they will break consistently so it's easy to find and fix the problem.

    48. nascheme commented on Dec 19, 2017

      @nascheme
      Member

      Welp, another day another attempt. As mentioned in the PR 4847, atexit is not the answer. If the raw/buffered file pair are part of a reference cycle and the GC cleans it before atexit runs, then the buffered data can get lost.

      I attempted to implement my weakref idea (i.e. raw file keeps a weakref to the buffered file, calls flush before the raw file gets closed). That doesn't work either because the GC clears the weakref before calling __del__.

      The attached patch "buffer_register_flush.txt" does seem to work. The downside is that it creates a reference cycle between the raw and buffered file objects. Perhaps that is not a problem since unless you call close() on the raw file, you will be leaking resources anyhow. In the patch, calling close() removes the reference cycle.

      I still feel like this is worth fixing, as ugly as the implementation is.

    49. self-assigned this
      on Dec 19, 2017
    50. pitrou commented on Dec 19, 2017

      @pitrou
      Member

      I think it would be quite disruptive to create a reference cycle each time open() is called. It may also break user scripts.

    51. nascheme commented on Dec 19, 2017

      @nascheme
      Member

      Yeah, I think you are correct. Currently files not part of reference cycles get properly flushed and closed by the reference counter. Implementing my "buffer_register_flush" patch would cause files to be closed only by the cyclic garbage collector (if not explicitly closed). That would mean a script that opens a large number of files could run out of file descriptors. Basically, we lose one of the main advantages of reference counting.

      Probably the GC should keep track of how many files are open and call collect() when a threshold is reached. Still, forcing every file to be collected only by the cyclic GC seems too ugly of a solution to this issue.

    52. pitrou commented on Dec 20, 2017

      @pitrou
      Member

      Here's another idea: have a new type field (a tp_something) that exposes the GC priority for this type. It could be an integer between -8 and 7, for example (0 being the default). Then tp_finalize would be called in sorted priority order.

      The tricky part of course is to make this acceptable performance-wise (i.e. avoid walking the collectable list 16 times, once for each priority level). For example by creating bucket lists for each priority level (only for objects with a non-NULL tp_finalize).

    53. Arusekk commented on Jan 24, 2018

      Arusekkmannequin
      Mannequin

      Since the issue seems to have been active lately, may I suggest my view on solving it.

      One solution that comes to my mind is to keep a weak reference to the file, and to register an atexit function per file (and to unregister it when the file is closed). Example concept-illustrating python code for the _pyio module:

      import atexit, weakref

      # ...

      class TextIOWrapper(TextIOBase):
          def __init__(self):
              # ...
              self._weakclose = operator.methodcaller('close')
              atexit.register(self._weakclose, weakref.proxy(self))
      # ...
      
          def close(self):
              atexit.unregister(self._weakclose)
              # and now actually close the file

      There is a possibility of a negative impact arising from the use of operator.methodcaller, because it may return something cached in future versions of python. There is also the issue of unregistering an atexit function during atexit processing. But the general idea seems to be simple and worth considering.

    54. nascheme commented on Jan 25, 2018

      @nascheme
      Member

      Using atexit is not the solution because the data can be lost even while the program is running, not just at interpreter shutdown. The problem occurs if the buffered file object and the underlying file object are both part of a reference cycle. Then, when the cycle is destroyed by the GC module, if the file __del__ is called before the buffer __del__, data is lost.

      So far, the best solution I've come up with is as follows: split the buffered file object into two objects, a wrapper object that will be returned from open() and an underlying object that holds the actual buffer. Make the underlying file object keep references to all the buffers that are using the file. When the file _del__ gets called, first flush all of the buffers and then close the file. Splitting the buffered file object is required so that we don't create reference cycles.

    55. njsmith commented on Jan 25, 2018

      @njsmith
      Contributor

      I attempted to implement my weakref idea (i.e. raw file keeps a weakref to the buffered file, calls flush before the raw file gets closed). That doesn't work either because the GC clears the weakref before calling __del__.

      This may be a bit of a left-field or too-big-a-hammer suggestion, but as far I can tell from this thread [1] it probably is technically possible to modify the GC to clear weakrefs after calling __del__. Nick wasn't a fan (he likes the invariant that weakrefs can't trigger a resurrection), but if all else fails it might be worth re-raising.

      You could add a secondary reference count on FileIO recording how many BufferedIO wrappers there are around it; then it's __del__ would skip calling close() if there are still BufferedIO wrappers, and BufferedIO.__del__ would decrement the reference count and close the underlying file if it hits zero and FileIO.__del__ had already been called.

      [1] https://mail.python.org/pipermail/python-dev/2016-October/146747.html

    56. changed the title [-]Built-in module _io can lose data from buffered files at exit[/-] [+]Built-in module _io can lose data from buffered files in reference cycles[/+] on Jan 27, 2018
    57. vstinner commented on Oct 21, 2019

      @vstinner
      Member

      I marked bpo-38548 as duplicate of this issue. Copy of msg355062:

      Consider the following program:

      f = open("out.txt", "w")
      f.write("abc\n")
      exit(0)

      Please note the absence of f.close().
      The documentation
      https://docs.python.org/3/tutorial/inputoutput.html#reading-and-writing-files
      says that you should use f.close() or with f = open(), but is not clear whether the program above without f.close() is guaranteed to write. The tutorial says:
      "If you don’t explicitly close a file, Python’s garbage collector will eventually destroy the object and close the open file for you, but the file may stay open for a while. Another risk is that different Python implementations will do this clean-up at different times."
      For me this sounds like even without f.close() the file is guaranteed to be written. If it is not guaranteed to be written, you should fix the documentation, if it is guaranteed to be written, then I will open another issue because the following program does not write into out.txt on my machine:

      from sympy.core import AtomicExpr
      class MyWeirdClass(AtomicExpr):
      	def __init__(self):
      		pass
      f = open("out.txt", "w")
      f.write("abc\n")
      exit(0)

      Note: sys.version is: "3.7.3 (default, Oct 7 2019, 12:56:13) \n[GCC 8.3.0]"

    58. methane commented on Nov 27, 2020

      @methane
      Member

      New changeset c8aaf71 by Volker-Weissmann in branch 'master':
      bpo-17852: Doc: Fix the tutorial about closing files (GH-23135)
      c8aaf71

    59. miss-islington commented on Nov 27, 2020

      @miss-islington
      Contributor

      New changeset 01fcde8 by Miss Islington (bot) in branch '3.8':
      bpo-17852: Doc: Fix the tutorial about closing files (GH-23135)
      01fcde8

    60. Volker-Weissmann commented on Nov 27, 2020

      Volker-Weissmannmannequin
      Mannequin

      The PR below fixed it:

      #23135

    61. miss-islington commented on Dec 1, 2020

      @miss-islington
      Contributor

      New changeset 4a44f53 by Miss Islington (bot) in branch '3.9':
      [3.9] bpo-17852: Doc: Fix the tutorial about closing files (GH-23135) (GH-23527)
      4a44f53

    62. transferred this issue fromon Apr 10, 2022
    63. gpshead commented on May 26, 2023

      @gpshead
      Member

      is there still an issue to be fixed here? if so re-open and explain or open a new issue. The docmentation was updated to highlight that unclose files can't be guaranteed to flush buffered data.

    64. nascheme commented on May 31, 2023

      @nascheme
      Member

      I think there is still an issue here, even though it is a low priority bug. It's good we document now that unclosed files could lose buffered data. However, it is bad, IMHO, that it works for 99%+ of the time but sometimes doesn't. I think that's worth fixing, if we can do it without too much pain.

      I created GH-105171 as a different approach to fix this.

    65. vstinner commented on Jun 1, 2023

      @vstinner
      Member

      Over last years, I made many enhancements to be able to log ResourceWarning as late as possible. But Python finalization is not deterministic and so some warnings cannot be logged when emitted "too late".

      My latest failed attempt to make the finalization more determistic: #86837

    66. serhiy-storchaka commented on Jun 6, 2023

      @serhiy-storchaka
      Member

      The root cause is the GC algorithm for breaking reference cycles. If we have a cycle:

      flowchart LR;
          A==>B;
          B==>C;
          C==>A;
          C-->TextIOWrapper;
          TextIOWrapper-->BufferedWriter;
          BufferedWriter-->FileIO;
      
      Loading

      then it will try to break it by breaking any reference, including references C-->TextIOWrapper, TextIOWrapper-->BufferedWriter and BufferedWriter-->FileIO, even if they do not lead to breaking the cycle. The only references which advance in reference cycle elimination are A-->B, B-->C and C-->A. If the algorithm be smarter, this would solve most of problems, not only with IO.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Metadata

    Metadata

    Assignees

    Labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions