bpo-17852: Maintain a list of BufferedWriter objects. Flush them on exit. #1908

nascheme · 2017-06-01T17:56:40Z

In Python 3, the buffer and the underlying file object are separate
and so the order in which objects are finalized matters. This is
unlike Python 2 where the file and buffer were a single object and
finalization was done for both at the same time. In Python 3, if
the file is finalized and closed before the buffer then the data in
the buffer is lost.

This change adds a doubly linked list of open file buffers. An atexit
hook ensures they are flushed before proceeding with interpreter
shutdown. This is addition does not remove the need to properly close
files as there are other reasons why buffered data could get lost during
finalization.

Initial patch by Armin Rigo.

https://bugs.python.org/issue17852

pitrou · 2017-06-02T18:44:53Z

Lib/_pyio.py

+
+import atexit, weakref
+
+_all_writers = weakref.WeakKeyDictionary()


We have WeakSet now :-)

pitrou · 2017-06-02T18:47:54Z

Modules/_io/bufferedio.c

+
+    /* a doubly-linked chained list of "buffered" objects that need to
+       be flushed when the process exits */
+    struct doubly_linked_s buffered_writers_list;


The nested struct makes for quirky pointer calculation in _PyIO_atexit_flush(). Why not simply:

typedef struct _buffered { // ... struct _buffered *next, *prev; } buffered;

pitrou · 2017-06-02T18:48:45Z

Modules/_io/bufferedio.c

+                                     offsetof(buffered, buffered_writers_list));
+        remove_from_linked_list(buf);
+        buffered_flush(buf, NULL);
+        PyErr_Clear();


Rather than silence errors, they can be displayed using PyErr_WriteUnraisable.

nascheme · 2017-06-05T17:30:38Z

Antoine, thanks for taking the time for the review. I have modified the code as you suggest.

nascheme · 2017-06-05T17:41:14Z

I have left PyErr_Clear(). Using PyErr_WriteUnraisable might be better except that we could get some mysterious errors where previously there were none. The unit test suite generates extra errors if we remove the PyErr_Clear() call.

$ ./python  ../Lib/test/regrtest.py test_io
Run tests sequentially
0:00:00 load avg: 0.18 [1/1] test_io
Exception ignored in: <_io.BufferedWriter name='<stderr>'>
Exception ignored in: <_io.BufferedWriter name='<stdout>'>
test test_io failed -- Traceback (most recent call last):
  File "/home/nas/src/cpython/Lib/test/test_io.py", line 3232, in test_create_at_shutdown_with_encoding
    self.assertFalse(err)
AssertionError: b"Exception ignored in: <_io.BufferedWriter name='<stderr>'>\nException ignored in: <_io.BufferedWriter name='<stdout>'>" is not false

test_io failed in 35 sec
```

pitrou · 2017-06-06T16:59:51Z

Lib/_pyio.py

+    # finalized before the buffered writer wrapping it then any buffered
+    # data will be lost.
+    for w in _all_writers:
+        w.flush()


Perhaps this should ignore errors, as the C version does?

I think that would be wise. flush() probably should not raise an error but if it does then not catching it would be bad. Do you think just try/except (bare) and just swallow it? I think that would be okay because we really don't expect flush() to raise an error and if it does, there is nothing much to do about it.

Common case for a flush error is a broken pipe or disk full. Try writing to /dev/full. It would be nice if this indicated an error, but I suspect it would be hard to do (related: https://bugs.python.org/issue5319).

In Python 3, the buffer and the underlying file object are separate and so the order in which objects are finalized matters. This is unlike Python 2 where the file and buffer were a single object and finalization was done for both at the same time. In Python 3, if the file is finalized and closed before the buffer then the data in the buffer is lost. This change adds a doubly linked list of open file buffers. An atexit hook ensures they are flushed before proceeding with interpreter shutdown. This is addition does not remove the need to properly close files as there are other reasons why buffered data could get lost during finalization. Initial patch by Armin Rigo.

…hem on exit. (#1908)" This reverts commit e38d12e.

…hem on exit. (#1908)" (#3337) This reverts commit e38d12e.

* 'master' of https://github.com/python/cpython: (32 commits) Conceptually, roots is a set. Also searching it as a set is a tiny bit faster (python#3338) bpo-31343: Include sys/sysmacros.h (python#3318) bpo-30102: Call OPENSSL_add_all_algorithms_noconf (python#3112) Prevent a few make suspicious warnings. (python#3341) Include additional changes to support blurbified NEWS (python#3340) Simplify NEWS entry to prevent suspicious warnings. (python#3339) bpo-31347: _PyObject_FastCall_Prepend: do not call memcpy if args might not be null (python#3329) Revert "bpo-17852: Maintain a list of BufferedWriter objects. Flush them on exit. (python#1908)" (python#3337) bpo-17852: Maintain a list of BufferedWriter objects. Flush them on exit. (python#1908) Fix terminology in comment and add more design rationale. (python#3335) Add comment to explain the implications of not sorting keywords (python#3331) bpo-31170: Update libexpat from 2.2.3 to 2.2.4 (python#3315) bpo-28411: Remove "modules" field from Py_InterpreterState. (python#1638) random_triangular: sqrt() is more accurate than **0.5 (python#3317) Travis: use ccache (python#3307) remove IRIX support (closes bpo-31341) (python#3310) Code clean-up. Remove unnecessary pre-increment before the loop starts. (python#3312) Regen Moduls/clinic/_ssl.c.h (pythonGH-3320) bpo-30502: Fix handling of long oids in ssl. (python#2909) Cache externals, depending on changes to PCbuild (python#3308) ...

…xit. (python#1908) * Maintain a list of BufferedWriter objects. Flush them on exit. In Python 3, the buffer and the underlying file object are separate and so the order in which objects are finalized matters. This is unlike Python 2 where the file and buffer were a single object and finalization was done for both at the same time. In Python 3, if the file is finalized and closed before the buffer then the data in the buffer is lost. This change adds a doubly linked list of open file buffers. An atexit hook ensures they are flushed before proceeding with interpreter shutdown. This is addition does not remove the need to properly close files as there are other reasons why buffered data could get lost during finalization. Initial patch by Armin Rigo. * Use weakref.WeakSet instead of WeakKeyDictionary. * Simplify buffered double-linked list types. * In _flush_all_writers(), suppress errors from flush(). * Remove NEWS entry, use blurb.

…hem on exit. (python#1908)" (python#3337) This reverts commit e38d12e.

the-knights-who-say-ni added the CLA signed label Jun 1, 2017

pitrou reviewed Jun 2, 2017

View reviewed changes

Lib/_pyio.py Outdated

import atexit, weakref

_all_writers = weakref.WeakKeyDictionary()

Copy link

Member

pitrou Jun 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have WeakSet now :-)

pitrou reviewed Jun 2, 2017

View reviewed changes

pitrou reviewed Jun 6, 2017

View reviewed changes

nascheme force-pushed the io_flush_atexit branch from 46475fb to 994bd31 Compare June 23, 2017 17:11

nascheme force-pushed the io_flush_atexit branch from 994bd31 to 5d0ca62 Compare September 4, 2017 19:54

nascheme added 5 commits September 4, 2017 16:02

Use weakref.WeakSet instead of WeakKeyDictionary.

c6c1bbd

Simplify buffered double-linked list types.

74731be

In _flush_all_writers(), suppress errors from flush().

61952f0

Remove NEWS entry, use blurb.

a98052f

nascheme force-pushed the io_flush_atexit branch from 5d0ca62 to a98052f Compare September 4, 2017 23:03

nascheme merged commit e38d12e into python:master Sep 5, 2017

nascheme added a commit that referenced this pull request Sep 5, 2017

Revert "bpo-17852: Maintain a list of BufferedWriter objects. Flush t…

51a2600

…hem on exit. (#1908)" This reverts commit e38d12e.

nascheme mentioned this pull request Sep 5, 2017

Revert "bpo-17852: Maintain a list of BufferedWriter objects. Flush them on exit." #3337

Merged

nascheme added a commit that referenced this pull request Sep 5, 2017

Revert "bpo-17852: Maintain a list of BufferedWriter objects. Flush t…

db56423

…hem on exit. (#1908)" (#3337) This reverts commit e38d12e.

GadgetSteve pushed a commit to GadgetSteve/cpython that referenced this pull request Sep 10, 2017

Revert "bpo-17852: Maintain a list of BufferedWriter objects. Flush t…

bba3d79

…hem on exit. (python#1908)" (python#3337) This reverts commit e38d12e.

arigo mannequin mentioned this pull request May 26, 2023

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-17852: Maintain a list of BufferedWriter objects. Flush them on exit. #1908

bpo-17852: Maintain a list of BufferedWriter objects. Flush them on exit. #1908

nascheme commented Jun 1, 2017 •

edited by bedevere-bot

Loading

pitrou Jun 2, 2017

pitrou Jun 2, 2017

pitrou Jun 2, 2017

nascheme commented Jun 5, 2017

nascheme commented Jun 5, 2017

pitrou Jun 6, 2017

nascheme Jun 6, 2017 •

edited

Loading

vadmium Jun 17, 2017


		import atexit, weakref

		_all_writers = weakref.WeakKeyDictionary()

bpo-17852: Maintain a list of BufferedWriter objects. Flush them on exit. #1908

bpo-17852: Maintain a list of BufferedWriter objects. Flush them on exit. #1908

Conversation

nascheme commented Jun 1, 2017 • edited by bedevere-bot Loading

pitrou Jun 2, 2017

Choose a reason for hiding this comment

pitrou Jun 2, 2017

Choose a reason for hiding this comment

pitrou Jun 2, 2017

Choose a reason for hiding this comment

nascheme commented Jun 5, 2017

nascheme commented Jun 5, 2017

pitrou Jun 6, 2017

Choose a reason for hiding this comment

nascheme Jun 6, 2017 • edited Loading

Choose a reason for hiding this comment

vadmium Jun 17, 2017

Choose a reason for hiding this comment

nascheme commented Jun 1, 2017 •

edited by bedevere-bot

Loading

nascheme Jun 6, 2017 •

edited

Loading