Skip to content
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.

Sign up for GitHub

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

Jump to bottom

bootstrap: write texts to a .tmp file first for atomicity #51816

Merged
merged 2 commits into from
Jul 12, 2018
Merged

bootstrap: write texts to a .tmp file first for atomicity #51816

merged 2 commits into from
Jul 12, 2018

Conversation

nodakai
Copy link
Contributor

@nodakai nodakai commented Jun 26, 2018

If you are using a hard-linked file as your config.toml, this change will affect the way other instances of the file is modified.
The original version would modify all other instances whereas the new version will leave others unchanged, reducing the ref count by one.

@rust-highfive
Copy link
Collaborator

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 26, 2018
@Mark-Simulacrum
Copy link
Member

r? @kennytm

kennytm
kennytm reviewed Jun 26, 2018
View reviewed changes
Copy link
Member

@kennytm kennytm left a comment •
edited
Loading

Choose a reason for hiding this comment

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

r=me with one nit.

src/bootstrap/configure.py Outdated
@@ -428,11 +429,18 @@ def configure_section(lines, config):
else:
configure_section(sections[section_key], section_config)

@contextlib.contextmanager
def output(filepath):
Copy link
Member

Choose a reason for hiding this comment

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

configure imports bootstrap, so just reuse bootstrap.output and remove this def.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point, fixed it accordingly.

@kennytm kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 26, 2018
@nodakai nodakai force-pushed the conf-py-tmpfile branch 2 times, most recently from de9d173 to 9eb3d5a Compare June 26, 2018 17:29
@kennytm
Copy link
Member

kennytm commented Jun 26, 2018

Hold on, I have a concern.

https://docs.python.org/2/library/os.html#os.rename

On Windows, if dst already exists, OSError will be raised even if it is a file; there may be no way to implement an atomic rename when dst names an existing file.

This means it can't really overwrite a file on Windows.

Calling os.remove isn't a complete fix.

https://docs.python.org/2/library/os.html#os.remove

On Windows, attempting to remove a file that is in use causes an exception to be raised

On Python 3.3+ we could use os.replace, but these Python scripts need to support both Python 2.x and 3.x.

Could you perform these fallback measures?

  1. If os.replace exists, use it
  2. Try to delete the original file before renaming.
  3. Try copy the content to the original file if all failed.

@nodakai
Copy link
Contributor Author

nodakai commented Jun 27, 2018

Crap, this thing totally slipped my mind, I've been away from Win32 these days...

I've pushed a better Win32 support as you suggested, but I'm not sure if the goal of this patch worths the code length anymore.

This is a WIP as I'm still confirming shutil.copyfile won't throw in this use case.

@alexcrichton
Copy link
Member

Could the file be removed, ignoring errors, prior to creation? I think that'd help avoid the platform-specific goop?

@stokhos
Copy link

stokhos commented Jul 6, 2018

Ping from triage, @nodakai we haven't heard from you for a while, will you have time to look into this PR in near future?

@nodakai
Copy link
Contributor Author

nodakai commented Jul 8, 2018

Just pushed smaller fixes; waiting for the CI...

@alexcrichton
Copy link
Member

Since we aren't really going for speed here, could remove + write always be used? It seems like that would be the most clear with no version or os checks

@nodakai
bootstrap: write texts to a .tmp file first for atomicity
bcb8a06 
If you are using a hard-linked file as your config.toml, this change will affect the way other instances of the file is modified.
The original version would modify all other instances whereas the new version will leave others unchanged, reducing the ref count by one.

Signed-off-by: NODA, Kai <nodakai@gmail.com>
@nodakai
bootstrap: our best to achieve atomic rename on Win32
97d0bc3 
This is a tricky operation to implement on Win32; see
https://ci.appveyor.com/project/nodakai/python-win-behavior

Signed-off-by: NODA, Kai <nodakai@gmail.com>
@nodakai
Copy link
Contributor Author

nodakai commented Jul 10, 2018

@alexcrichton Indeed there's little to be gained from sticking to the really-atomic overwrite. Now we have just two cases.

@kennytm kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 10, 2018
@alexcrichton
Copy link
Member

The aspect of windows holding a file open can probably be skipped as well, right? This is just a configuration file and doesn't really have a lot of windows programs holding it open I think?

@nodakai
Copy link
Contributor Author

nodakai commented Jul 10, 2018

@alexcrichton Your text editor could be one, when you are watching what the generated file looks like?

@alexcrichton
Copy link
Member

Do text editors really keep files open? I'm under the impression they only hold it briefly open while opening and closing files

@kennytm
Copy link
Member

kennytm commented Jul 11, 2018

At least for notepad.exe it does keep the file open.

@alexcrichton
Copy link
Member

Eh I'll leave this up to you @kennytm

@kennytm
Copy link
Member

kennytm commented Jul 11, 2018

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jul 11, 2018

📌 Commit 97d0bc3 has been approved by kennytm

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 11, 2018
kennytm added a commit to kennytm/rust that referenced this pull request Jul 12, 2018
@kennytm
Rollup merge of rust-lang#51816 - nodakai:conf-py-tmpfile, r=kennytm
c87e3fe 
bootstrap: write texts to a .tmp file first for atomicity

If you are using a hard-linked file as your config.toml, this change will affect the way other instances of the file is modified.
The original version would modify all other instances whereas the new version will leave others unchanged, reducing the ref count by one.
@kennytm kennytm mentioned this pull request Jul 12, 2018
Merged
bors added a commit that referenced this pull request Jul 12, 2018
@bors
Auto merge of #52303 - kennytm:rollup, r=kennytm
7db82cc 
Rollup of 9 pull requests

Successful merges:

 - #51816 (bootstrap: write texts to a .tmp file first for atomicity)
 - #51912 (impl Clone for Box<CStr>, Box<OsStr>, Box<Path>)
 - #52164 (use proper footnote syntax for references)
 - #52220 (Deny bare trait objects in `src/bootstrap`)
 - #52276 (rustc: Verify #[proc_macro] is only a word)
 - #52277 (Uncapitalize "If")
 - #52287 (Deny bare trait objects in src/librustc_resolve)
 - #52295 (Deny bare trait objects in src/libsyntax_ext)
 - #52298 (make reference to dirs crate clickable in terminals)

Failed merges:

r? @ghost
@bors bors merged commit 97d0bc3 into rust-lang:master Jul 12, 2018
@nodakai nodakai deleted the conf-py-tmpfile branch August 1, 2021 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kennytm kennytm kennytm left review comments

Assignees

@kennytm kennytm

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@nodakai @rust-highfive @Mark-Simulacrum @kennytm @alexcrichton @stokhos @bors