Skip to content

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

Merged
terryjreedy merged 2 commits intopython:masterfrom
gvanrossum:flush-fsync-idle
May 13, 2019
Merged

bpo-36807: When saving a file in IDLE, call flush and fsync#13102
terryjreedy merged 2 commits intopython:masterfrom
gvanrossum:flush-fsync-idle

Conversation

@gvanrossum
Copy link
Copy Markdown
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
Copy Markdown
Member Author

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

@dhalbert
Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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

@miss-islington
Copy link
Copy Markdown
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
Copy Markdown
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
…-13102)

(cherry picked from commit 4f098b3)

Co-authored-by: Guido van Rossum <guido@python.org>
@bedevere-bot
Copy link
Copy Markdown

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

Comment thread Misc/NEWS.d/next/IDLE/2019-05-05-16-27-53.bpo-13102.AGNWYJ.rst
@gvanrossum gvanrossum deleted the flush-fsync-idle branch May 13, 2019 14:37
@gvanrossum
Copy link
Copy Markdown
Member Author

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

@gvanrossum
Copy link
Copy Markdown
Member Author

Typo fix is here: #13284

@miss-islington
Copy link
Copy Markdown
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
…-13102)

(cherry picked from commit 4f098b3)

Co-authored-by: Guido van Rossum <guido@python.org>
@bedevere-bot
Copy link
Copy Markdown

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