Skip to content

bpo-36807: When saving a file in IDLE, call flush and fsync #13102

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

Merged
merged 2 commits into from
May 13, 2019

Conversation

gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented May 5, 2019

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. And I think this PR fixes the issue.

I've tested that this doesn't break things, but I don't have the right
cable around so I cannot test this with the hardware handed out to
PyCon attendees until Wednesday at the earliest.

https://bugs.python.org/issue36807

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.  And I *think* this PR fixes the issue.

I've tested that this doesn't break things, but I don't have the right
cable around so I cannot test this with the hardware handed out to
PyCon attendees until Wednesday at the earliest.
@gvanrossum gvanrossum changed the title [idle] When saving a file, call flush and fsync bpo-36807 When saving a file, call flush and fsync May 5, 2019
@gvanrossum gvanrossum changed the title bpo-36807 When saving a file, call flush and fsync bpo-36807 When saving a file in IDLE, call flush and fsync May 5, 2019
@gvanrossum gvanrossum changed the title bpo-36807 When saving a file in IDLE, call flush and fsync bpo-36807: When saving a file in IDLE, call flush and fsync May 5, 2019
@terryjreedy terryjreedy added needs backport to 3.7 type-bug An unexpected behavior, bug, or error labels May 6, 2019
@gvanrossum
Copy link
Member Author

@dhalbert Can you test this on Windows? Thanks for finding this issue in the first place!

@dhalbert
Copy link

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!

@gvanrossum
Copy link
Member Author

@terryjreedy I personally think this is good to go in. The standard builtin open() always returns an object with flush() and fileno()methods, andos.fsync()` exists on Linux, Mac and Windows. I have no idea whether it would be safer to print a message (how and where?) or just ignore errors -- perhaps the right thing to do is just to let IDLE crash in the extremely rare case where a call does fail?

@terryjreedy
Copy link
Member

I am most worried about an exception from pulling the usb plug, in which case ignoring it should be fine. I will merge this tomorrow with try:...except:pass added.

@gvanrossum
Copy link
Member Author

I don't see why anything extra is needed. There's already a try/except OSError that puts up a dialog box. I just tried this on Mac (edit file, remove USB plug, try to save) and it worked as expected.

@terryjreedy terryjreedy merged commit 4f098b3 into python:master May 13, 2019
@bedevere-bot
Copy link

@terryjreedy: Please replace # with GH- in the commit message next time. Thanks!

@miss-islington
Copy link
Contributor

Thanks @gvanrossum for the PR, and @terryjreedy for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Thanks @gvanrossum for the PR, and @terryjreedy for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 13, 2019
@bedevere-bot
Copy link

GH-13280 is a backport of this pull request to the 3.7 branch.

@gvanrossum gvanrossum deleted the flush-fsync-idle branch May 13, 2019 14:37
@gvanrossum
Copy link
Member Author

OK, I'll send a fix for the NEW typo.

@gvanrossum
Copy link
Member Author

Typo fix is here: #13284

@miss-islington
Copy link
Contributor

Thanks @gvanrossum for the PR, and @terryjreedy for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 13, 2019
@bedevere-bot
Copy link

GH-13288 is a backport of this pull request to the 3.7 branch.

terryjreedy pushed a commit that referenced this pull request May 13, 2019
terryjreedy added a commit to terryjreedy/cpython that referenced this pull request May 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants