classification
Title: IDLE doesn't call os.fsync()
Type: behavior Stage: resolved
Components: IDLE Versions: Python 3.8, Python 3.7, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: terry.reedy Nosy List: dhalbert, gvanrossum, serhiy.storchaka, terry.reedy
Priority: normal Keywords: patch

Created on 2019-05-05 20:22 by gvanrossum, last changed 2019-05-13 23:52 by gvanrossum. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 13102 closed gvanrossum, 2019-05-05 20:23
PR 13280 closed miss-islington, 2019-05-13 13:07
PR 13284 closed gvanrossum, 2019-05-13 14:38
PR 13288 merged miss-islington, 2019-05-13 16:10
PR 13293 merged terry.reedy, 2019-05-13 17:36
Messages (19)
msg341474 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2019-05-05 20:22
This came up during today's final PyCon keynote -- IDLE was called out
as one of the two editors *not* to use when live-coding on Adafruit's
Circuit Playground Express (https://www.adafruit.com/product/3333).

I *think* that the problem referred to is that IDLE doesn't guarantee
that the bits are actually flushed to disk -- they may linger in the
OS filesystem cache.
msg341480 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2019-05-06 01:27
If/when you accept this we should also backport it as far as we can.
msg341486 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2019-05-06 06:30
This seems like a good idea.   I am assuming that open('somename', ...) always result in a file object with .fileno defined.

Although I am mostly not touching 2.7, this should be easy to do.  It will have to be done manually since iomenu was then IOBinding.
msg341487 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2019-05-06 07:23
OS and disk interaction is not something I know a lot about.  I don't know how long different OSes take to write things out by themselves, and therefore how much real danger there is of loosing data.  However, IDLE is used in places where power is less reliable than it is for me, and if not doing this makes IDLE look bad, and if it does not noticeably delay running a file (and I expect not), then it seems like a good idea.

Digging further, Kurt Kaiser added f.flush in 3/19/2006.  On 2013-08-04, for #18151, Serhiy submitted a patch for 3.3 with the comment "Here is a patch which replace the "open ... close" idiom to the "with open" idiom in IDLE."  It replaced
            f = open(filename, "wb")
            f.write(chars)
            f.flush()
            f.close()
with
            with open(filename, "wb") as f:
                f.write(chars)

I have no idea why f.flush was deleted.  An oversight?  There is no discussion on the issue. I merged Serhiy's patch and backported to 2.7.
msg341490 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2019-05-06 07:42
The io doc says for IOBase flush()
    Flush the write buffers of the stream if applicable. This does nothing for read-only and non-blocking streams.

and for BufferedWriter flush()
    Force bytes held in the buffer into the raw stream. A BlockingIOError should be raised if the raw stream blocks.

On 3.x, open(filename, "wb"), used in writefile(), returns a BufferedWriter.  So it seems than an exception is possible, which would crash IDLE without try-except.

Serhiy, please read the previous message(s).  Do you remember if you intended to remove the f.flush in writefile(), which Guido proposes to restore?
msg341491 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-05-06 08:06
f.flush() was removed because it is redundant if followed by f.close(). f.close() calls f.flush() internally. f.close() is now called implicitly in __exit__() when exit from the with statement.

Guido restores f.flush() because he needs to flush the buffer of the buffered stream before calling os.fsync(f.fileno()). And f.fileno() can be used only for not closed file. So this change looks reasonable to me.

I do not know what are the troubles with Adafruit's
Circuit Playground Express. I think that flushing the OS filesystem cache is the work of the OS itself, and lingering the data in the OS-wide cache should not affect userspace. But maybe there are specific circumstences, for example if IDLE and the consumer of Python source files are ran on different machines.
msg341494 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2019-05-06 12:38
That board implements a USB filesystem that you plug into a computer (via a cable). The control software on the board watches this USB filesystem, and when the "code.py" file changes it reloads that file and executes it with CircuitPython (Adafruit's fork of MicroPython).

So the recommended workflow for the user is: edit the code to program a LED blinking pattern (for example); save the file; watch the LEDs on the board blink in the programmed pattern. Repeat with other changes. Endless fun.

Where IDLE currently doesn't cooperate is that after a save operation, the OS kernel doesn't immediately flush the bytes to the USB filesystem, so the board doesn't see the changes to the file until the OS cache gets flushed at the kernel's whim. (The board uses very low level fs operations because it's an 8-bit microprocessor with very little memory and therefore has very primitive software.)

The os.fsync() call ought to fix it by forcing the kernel to flush the bytes to the USB filesystem right away. This also helps when the user saves the file and immediately pulls out the USB cable. The OS will issue a warning in an attempt to train users to "Eject" first, but beginners are prone to making this mistake repeatedly.

Hope this helps. You should really watch @nnja's keynote once it goes online, it was really cool. :-)
msg341680 - (view) Author: Dan Halbert (dhalbert) Date: 2019-05-07 04:03
I'm one of the CircuitPython core devs. This issue is OS-dependent: Windows and Linux don't necessarily write data and metadata out to USB drives promptly. The problem is particularly acute for FAT12 filesystems on Windows, which are typically 16MB or smaller: Windows can take up to 90 seconds to flush, due to a driver bug or perhaps some ancient concerns about floppy drives. MacOS doesn't have this issue.

See https://superuser.com/questions/1197897/windows-delays-writing-fat-table-on-small-usb-drive-despite-quick-removal/1203781 for details. We have contacted Microsoft about this but getting it fixed is a long-term and uncertain process.

I'll test on Windows when I return from PyCon. Thank you for the prompt fix!
msg341850 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2019-05-08 04:06
I watched the talk by Nina Zakharenko at https://www.youtube.com/watch?v=35mXD40SvXM.  Since I love being able to save, compile, and run a file with one keystroke, I appreciate people wanting to do the equivalent with CircuitExpress and Adafruit.  In IDLE, print() output would appear in IDLE's Shell unless sys.stdout were replaced.

Dan's note suggests that flush/fsynch would be more generally useful than for the board.  I am inclined to add try: except: around the calls, and perhaps display a message "File was not saved" + traceback.

Dan, slightly OT, but I am curious whether one can access USB ports (in a system-dependent manner) directly from python code via os.system and ctypes?
msg341851 - (view) Author: Dan Halbert (dhalbert) Date: 2019-05-08 04:18
>Dan, slightly OT, but I am curious whether one can access USB ports (in a system-dependent manner) directly from python code via os.system and ctypes?

Do you mean from CircuitPython? The USB impplementation provides HID keyboard, mouse, and gamepad devices, CDC (serial), MIDI, and MSC (CIRCUITPY). We don't provide ctypes (though MicroPython does), but there are native modules that provide access to these devices. We will are planning to add user-defined USB descriptors.

Also, we provide some enhancements in a CircuitPython-specific module to peek for input on CDC, so you can know when to call `input()`, without using the usual OS-dependent Python tricks for this.

Right now REPL input/output is mixed with user input, as you mention, but we're considering adding a second serial CDC port so you'll be able to have a second serial channel and avoid colliding with the REPL.

I'd be happy to discuss this kind of stuff with you further, by email or whatever other mechanism you have in mind, like some appropriate python mailing list or our own chat servers, etc.
msg342274 - (view) Author: Dan Halbert (dhalbert) Date: 2019-05-13 00:43
Fix tested and works! Comment from PR duplicated here.

I tested this fix by editing the 3.7.3 IDLE code by hand, and editing this test program as code.py on a CIRCUITPY drive on Windows 10:

import time
i = 0
while True:
    print(i)
    i += 1
    print("""\
123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890
123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890
""")
    time.sleep(1)

I typically test write flushing by editing a short program like this, and duplicating the print lines so that the size of the program grows by >512 bytes. This forces a rewrite of the FAT12 information because the program has increased in size by one or more filesystem blocks. Then I shrink it back down again. If file flushing doesn't happen immediately, the program will often throw a syntax error (because it's missing some blocks), or CircuitPython will get an I/O error, which is what happened before I patched IDLE.

So this looks good!
msg342275 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2019-05-13 00:52
@Terry please see my comment on the PR.
msg342313 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2019-05-13 12:31
New changeset 4f098b35f58e911639f8e9adc393d5cf5c792e7f by Terry Jan Reedy (Guido van Rossum) in branch 'master':
bpo-36807: When saving a file in IDLE, call flush and fsync (#13102)
https://github.com/python/cpython/commit/4f098b35f58e911639f8e9adc393d5cf5c792e7f
msg342353 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2019-05-13 16:02
When 3.x is done, I will do 2.7.
msg342360 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2019-05-13 16:58
New changeset 68a11b6e6a73cd2e9b49e5df9973877b2fed6277 by Terry Jan Reedy (Miss Islington (bot)) in branch '3.7':
[3.7] bpo-36807: When saving a file in IDLE, call flush and fsync (GH-13102) (#13288)
https://github.com/python/cpython/commit/68a11b6e6a73cd2e9b49e5df9973877b2fed6277
msg342366 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2019-05-13 17:38
PR 13102 is original patch for master; it was merged, not closed.
PR 13284 fixed blurb for master.
PR 13280 backport for 3.7 was closed trying to restart bogus failure; can't do that for backports.
PR 13288 is second backport; only optional Azure Pipilines failed.
PR 13293 is manual 2.7 backport.
msg342397 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2019-05-13 22:29
New changeset 353f8d2282b1a48376f8813fd1170445a0cda873 by Terry Jan Reedy in branch '2.7':
[2.7] bpo-36807: When saving a file in IDLE, call flush and fsync (GH-13102) (GH-13293)
https://github.com/python/cpython/commit/353f8d2282b1a48376f8813fd1170445a0cda873
msg342400 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2019-05-13 23:47
> Do you mean from CircuitPython?

No, I was asking about regular Python on Windows.  I was not clear to me whether any of the software that Nina ran (other than possibly Adafruit USB drivers) was from Adafruit.  I am now guessing that is was all 3rd party stuff and that IDLE will now be an alternative for part of what she used.

I was in part wondering whether there were other simple (and allowable) changes to IDLE that might make it more useful for this type of application, but I will leave that sit for now.

I answered at least some of my questions about USB and the Adafruit hardware and the rest of what you wrote with my search bar.
msg342401 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2019-05-13 23:52
Thanks all!
History
Date User Action Args
2019-05-13 23:52:32gvanrossumsetmessages: + msg342401
2019-05-13 23:47:53terry.reedysetstatus: open -> closed
resolution: fixed
messages: + msg342400

stage: patch review -> resolved
2019-05-13 22:29:18terry.reedysetmessages: + msg342397
2019-05-13 17:38:30terry.reedysetmessages: + msg342366
2019-05-13 17:36:56terry.reedysetpull_requests: + pull_request13204
2019-05-13 16:58:48terry.reedysetmessages: + msg342360
2019-05-13 16:10:28miss-islingtonsetpull_requests: + pull_request13198
2019-05-13 16:02:33terry.reedysetmessages: + msg342353
2019-05-13 14:38:48gvanrossumsetpull_requests: + pull_request13191
2019-05-13 13:07:22miss-islingtonsetpull_requests: + pull_request13186
2019-05-13 12:31:33terry.reedysetmessages: + msg342313
2019-05-13 00:52:30gvanrossumsetmessages: + msg342275
2019-05-13 00:44:00dhalbertsetmessages: + msg342274
2019-05-08 04:18:55dhalbertsetmessages: + msg341851
2019-05-08 04:06:46terry.reedysetmessages: + msg341850
2019-05-07 04:03:05dhalbertsetnosy: + dhalbert
messages: + msg341680
2019-05-06 12:38:01gvanrossumsetmessages: + msg341494
2019-05-06 08:06:40serhiy.storchakasetmessages: + msg341491
2019-05-06 07:42:44terry.reedysetnosy: + serhiy.storchaka
messages: + msg341490
2019-05-06 07:23:58terry.reedysetmessages: + msg341487
2019-05-06 06:30:35terry.reedysetmessages: + msg341486
versions: + Python 2.7, - Python 3.6
2019-05-06 01:27:58gvanrossumsetmessages: + msg341480
2019-05-05 20:23:02gvanrossumsetkeywords: + patch
pull_requests: + pull_request13015
2019-05-05 20:22:26gvanrossumcreate