Skip to content

/cpython

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

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

Merged
merged 2 commits into from May 13, 2019
+3 −0

Conversation

Reviewers

@dhalbert dhalbert

@serhiy-storchaka serhiy-storchaka

@terryjreedy terryjreedy

Assignees
No one assigned
Labels
CLA signed type-bugfix
Projects
None yet
Milestone
No milestone
7 participants
@gvanrossum @dhalbert @terryjreedy @bedevere-bot @miss-islington @serhiy-storchaka @the-knights-who-say-ni
@gvanrossum
Copy link
Member

commented May 5, 2019 •
edited by bedevere-bot

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

@gvanrossum gvanrossum requested a review from terryjreedy as a code owner May 5, 2019

@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label May 5, 2019

@bedevere-bot bedevere-bot added the awaiting core review label May 5, 2019

@gvanrossum
[idle] When saving a file, call flush and fsync 
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.
Loading status checks…
0dfacce

@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

@gvanrossum
Add blurb
Loading status checks…
4108393

@terryjreedy terryjreedy added needs backport to 3.7 type-bugfix labels May 6, 2019

@gvanrossum

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented May 6, 2019

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

This comment has been minimized.

Sign in to view
Copy link

commented May 13, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented May 13, 2019

@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

This comment has been minimized.

Sign in to view
Copy link
Member

commented May 13, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented May 13, 2019

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.
@serhiy-storchaka

serhiy-storchaka approved these changes May 13, 2019
View changes

@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels May 13, 2019

@terryjreedy terryjreedy merged commit 4f098b3 into python:master May 13, 2019
5 checks passed

5 checks passed

Azure Pipelines PR #20190505.44 succeeded
Details
bedevere/issue-number Issue number 36807 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bedevere-bot

This comment has been minimized.

Sign in to view
Copy link

commented May 13, 2019

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

This comment has been minimized.

Sign in to view
Copy link

commented May 13, 2019

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

@bedevere-bot bedevere-bot removed the awaiting merge label May 13, 2019

@terryjreedy terryjreedy added needs backport to 3.7 and removed needs backport to 3.7 labels May 13, 2019

@miss-islington

This comment has been minimized.

Sign in to view
Copy link

commented May 13, 2019

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

miss-islington added a commit to miss-islington/cpython that referenced this pull request May 13, 2019

@gvanrossum @miss-islington
bpo-36807: When saving a file in IDLE, call flush and fsync (pythonGH… 
…-13102)

(cherry picked from commit 4f098b3)

Co-authored-by: Guido van Rossum <guido@python.org>
3629fed
@bedevere-bot

This comment has been minimized.

Sign in to view
Copy link

commented May 13, 2019

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

@bedevere-bot bedevere-bot removed the needs backport to 3.7 label May 13, 2019

@dhalbert

dhalbert reviewed May 13, 2019
View changes

Show resolved Hide resolved Misc/NEWS.d/next/IDLE/2019-05-05-16-27-53.bpo-13102.AGNWYJ.rst

@gvanrossum gvanrossum deleted the gvanrossum:flush-fsync-idle branch May 13, 2019

@gvanrossum

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented May 13, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented May 13, 2019

Typo fix is here: #13284

@terryjreedy terryjreedy added the needs backport to 3.7 label May 13, 2019

@miss-islington

This comment has been minimized.

Sign in to view
Copy link

commented May 13, 2019

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

miss-islington added a commit to miss-islington/cpython that referenced this pull request May 13, 2019

@gvanrossum @miss-islington
bpo-36807: When saving a file in IDLE, call flush and fsync (pythonGH… 
…-13102)

(cherry picked from commit 4f098b3)

Co-authored-by: Guido van Rossum <guido@python.org>
296c29a

@bedevere-bot bedevere-bot removed the needs backport to 3.7 label May 13, 2019

@bedevere-bot

This comment has been minimized.

Sign in to view
Copy link

commented May 13, 2019

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

terryjreedy added a commit that referenced this pull request May 13, 2019

@miss-islington @terryjreedy
[3.7] bpo-36807: When saving a file in IDLE, call flush and fsync (GH… 
…-13102) (#13288)

Includes blurb fix in 85c69d5
(cherry picked from commit 4f098b3)
Loading status checks…
68a11b6

terryjreedy added a commit to terryjreedy/cpython that referenced this pull request May 13, 2019

@terryjreedy
[2.7] bpo-36807: When saving a file in IDLE, call flush and fsync (py… 
…thonGH-13102)
8da8596

terryjreedy added a commit that referenced this pull request May 13, 2019

@terryjreedy
[2.7] bpo-36807: When saving a file in IDLE, call flush and fsync (GH… 
…-13102) (GH-13293)
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
353f8d2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.