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

fix bug: corruption when cargo killed while writing #12744

Merged
merged 1 commit into from
Oct 2, 2023
Merged

fix bug: corruption when cargo killed while writing #12744

merged 1 commit into from
Oct 2, 2023

Conversation

tompscanlan
Copy link
Contributor

@tompscanlan tompscanlan commented Sep 26, 2023 •
edited
Loading

What does this PR try to resolve?

fix #11386, superseding #12362

How should we test and review this PR?

Added unit test showing basic equivalency to existing write(path, content). Full test suite should exercise write.
Added tests for cargo add and remove. These are timing tests, so take a bit of time to run. 5-10s each. They may not fail every time, but do so regularly. Making the change to these two writes seems to prevent me from failing these tests at all.

Additional information

This uses tempfile::persist which was an existing dependency. atomicwrites crate, an alternative option for this fix, indicates tempfile::persist is the same thing. Since we already use tempfile as a dep, I stuck with that.

@rustbot
Copy link
Collaborator

rustbot commented Sep 26, 2023

r? @ehuss

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

@rustbot rustbot added A-build-scripts Area: build.rs scripts A-cache-messages Area: caching of compiler messages A-cfg-expr Area: Platform cfg expressions A-cli Area: Command-line interface, option parsing, etc. A-manifest Area: Cargo.toml issues A-rebuild-detection Area: rebuild detection and fingerprinting Command-fix Command-new Command-remove Command-vendor S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 26, 2023
@ehuss
Copy link
Contributor

ehuss commented Sep 26, 2023

@epage or @weihanglo, can either of you take assignment on this?

@epage
Copy link
Contributor

epage commented Sep 26, 2023

r? epage

@rustbot rustbot assigned epage and unassigned ehuss Sep 26, 2023
@tompscanlan
Copy link
Contributor Author

If it helps. I can write a test that makes write fail and shows no failure under write_atomic. Just let me know what you'd like to see.

@marcospb19 marcospb19 mentioned this pull request Sep 27, 2023
Closed
@tompscanlan
Copy link
Contributor Author

tompscanlan commented Sep 27, 2023 •
edited
Loading

To make discussion more productive, I added a test that is failing around write() when we use cargo remove dep. The same results should apply to any command using write(). Replacing the write() with write_atomic() makes this failing test pass.

I recommend we switch to the write_atomic(), but if occasionally zeroing out a cargo.toml is acceptable, then we should close this issue as a non-bug.

epage
epage reviewed Sep 29, 2023
View reviewed changes
tests/testsuite/death.rs Outdated Show resolved Hide resolved
@epage
Copy link
Contributor

epage commented Sep 29, 2023

Would you be up for cleaning up the commits for how you'd want them communicating out this change?

@tompscanlan
Copy link
Contributor Author

Thanks for the feedback, and yes, I'll be cleaning and adding one more test for the "add" command.

epage
epage reviewed Sep 30, 2023
View reviewed changes
tests/testsuite/death.rs Outdated Show resolved Hide resolved
epage
epage reviewed Sep 30, 2023
View reviewed changes
tests/testsuite/death.rs Outdated Show resolved Hide resolved
epage
epage reviewed Sep 30, 2023
View reviewed changes
tests/testsuite/death.rs Outdated Show resolved Hide resolved
epage
epage reviewed Sep 30, 2023
View reviewed changes
tests/testsuite/death.rs Outdated Show resolved Hide resolved
@epage
Copy link
Contributor

epage commented Sep 30, 2023

Thanks for continuing on this and sorry for the initial delay before I got to reviewing this!

@tompscanlan
Copy link
Contributor Author

No worries. I know ya'll are swamped. Thanks for steering. I may be away from keyboard for several days... will return to finish.

@tompscanlan
Copy link
Contributor Author

I think I'm done unless there's feedback on this change. Let me know @epage , thanks!

@epage
Copy link
Contributor

epage commented Oct 1, 2023

@tompscanlan sounded like you planned to clean up the commits. Were you still planning to do that?

@tompscanlan
Copy link
Contributor Author

I think I'm done @epage, unless we want to trim of the tests. I might take a few days to respond.

@epage
Copy link
Contributor

epage commented Oct 2, 2023

Thanks!

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 2, 2023

📌 Commit aee8c75 has been approved by epage

It is now in the queue for this repository.

@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 Oct 2, 2023
@bors
Copy link
Collaborator

bors commented Oct 2, 2023

⌛ Testing commit aee8c75 with merge 9a94183...

@bors
Copy link
Collaborator

bors commented Oct 2, 2023

☀️ Test successful - checks-actions
Approved by: epage
Pushing 9a94183 to master...

@bors bors merged commit 9a94183 into rust-lang:master Oct 2, 2023
20 checks passed
@tompscanlan tompscanlan deleted the atomic-write branch October 2, 2023 20:19
@weihanglo weihanglo mentioned this pull request Oct 4, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 4, 2023
@bors
Auto merge of rust-lang#116406 - weihanglo:update-cargo, r=weihanglo
dd513e1 
Update cargo

9 commits in 59596f0f31a94fde48b5aa7e945cd0b7ceca9620..794d0a82547f3081044c0aca7b6083733ce51344
2023-09-29 19:29:17 +0000 to 2023-10-03 23:19:33 +0000
- Prep for automating MSRV management (rust-lang/cargo#12767)
- chore(deps): update rust crate itertools to 0.11.0 (rust-lang/cargo#12759)
- fix bug: corruption when cargo killed while writing (rust-lang/cargo#12744)
- Disable custom_target::custom_bin_target on windows-gnu (rust-lang/cargo#12763)
- chore(deps): update compatible (rust-lang/cargo#12757)
- Add more missing `strip` info to docs. (rust-lang/cargo#12754)
- chore(deps): update actions/checkout action to v4 (rust-lang/cargo#12762)
- chore(deps): update rust crate cargo_metadata to 0.18.0 (rust-lang/cargo#12758)
- fix(test): Add back in newlines to diffs (rust-lang/cargo#12753)

r? ghost
@ehuss ehuss added this to the 1.75.0 milestone Oct 11, 2023
@calavera calavera mentioned this pull request Oct 14, 2023
Closed
@tompscanlan tompscanlan restored the atomic-write branch October 15, 2023 13:10
@epage epage mentioned this pull request Oct 16, 2023
Merged
@tompscanlan tompscanlan deleted the atomic-write branch November 17, 2023 16:57
@lkantola lkantola mentioned this pull request Feb 20, 2024
Open
epage added a commit to epage/cargo that referenced this pull request Apr 12, 2024
@epage
test: Remove add/remove death tests
7b30644 
Seeing recent fialures on Windows
- rust-lang#13748
- rust-lang#13738
- rust-lang#13740

and maybe more

The test was added in rust-lang#12744.  It seems of limited utility because there
are innumerable ways of adding new writes that aren't atomic and we
can't test for them all.
Even this case, its limited.

See also https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/Flaky.20test.3A.20.20-death.3A.3Akill_cargo_add_never_corrupts_cargo/near/432979594
@epage epage mentioned this pull request Apr 12, 2024
Merged
bors added a commit that referenced this pull request Apr 12, 2024
@bors
Auto merge of #13750 - epage:death, r=weihanglo
f7b713d 
test: Remove add/remove death tests

Seeing recent fialures on Windows
- #13748
- #13738
- #13740

and maybe more

The test was added in #12744.  It seems of limited utility because there are innumerable ways of adding new writes that aren't atomic and we can't test for them all.
Even this case, its limited.

See also https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/Flaky.20test.3A.20.20-death.3A.3Akill_cargo_add_never_corrupts_cargo/near/432979594
epage added a commit to epage/cargo that referenced this pull request Apr 12, 2024
@epage
test: Remove add/remove death tests
30efa8d 
Seeing recent fialures on Windows
- rust-lang#13748
- rust-lang#13738
- rust-lang#13740

and maybe more

The test was added in rust-lang#12744.  It seems of limited utility because there
are innumerable ways of adding new writes that aren't atomic and we
can't test for them all.
Even this case, its limited.

See also https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/Flaky.20test.3A.20.20-death.3A.3Akill_cargo_add_never_corrupts_cargo/near/432979594
bors added a commit that referenced this pull request Apr 12, 2024
@bors
Auto merge of #13750 - epage:death, r=weihanglo
48eca1b 
test: Remove add/remove death tests

Seeing recent fialures on Windows
- #13748
- #13738
- #13740

and maybe more

The test was added in #12744.  It seems of limited utility because there are innumerable ways of adding new writes that aren't atomic and we can't test for them all.
Even this case, its limited.

See also https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/Flaky.20test.3A.20.20-death.3A.3Akill_cargo_add_never_corrupts_cargo/near/432979594
@weihanglo weihanglo mentioned this pull request May 10, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@epage epage epage left review comments

@marcospb19 marcospb19 marcospb19 left review comments

Assignees

@epage epage

Labels
A-build-scripts Area: build.rs scripts A-cache-messages Area: caching of compiler messages A-cfg-expr Area: Platform cfg expressions A-cli Area: Command-line interface, option parsing, etc. A-manifest Area: Cargo.toml issues A-rebuild-detection Area: rebuild detection and fingerprinting Command-fix Command-new Command-remove Command-vendor S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.75.0
Development

Successfully merging this pull request may close these issues.

cargo add and ctrl + c
6 participants
@tompscanlan @rustbot @ehuss @epage @bors @marcospb19