Move the Windows remove_dir_all impl into a module and make it more race resistant #129800

ChrisDenton · 2024-08-31T02:52:49Z

This attempts to make the Windows implementation of remove_dir_all easier to understand and work with by separating out different concerns into their own functions. The code is mostly the same as before just moved around. There are some changes to make it more robust against races (e.g. two calls to remove_dir_all running concurrently). The module level comment explains the issue.

try-job: x86_64-msvc
try-job: i686-msvc

rustbot · 2024-09-01T17:42:37Z

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

ChrisDenton · 2024-09-01T23:04:31Z

I've enabled the test so I'll make extra sure it works in CI.

@bors try

Move the Windows remove_dir_all impl into a module and make it more race resistant This attempts to make the Windows implementation of `remove_dir_all` easier to understand and work with by separating out different concerns into their own functions. The code is mostly the same as before just moved around. There are some changes to make it more robust against races (e.g. two calls to `remove_dir_all` running concurrently). The module level comment explains the issue. try-job: x86_64-msvc try-job: i686-msvc

bors · 2024-09-01T23:05:43Z

⌛ Trying commit fca4020 with merge 0de7b4a...

bors · 2024-09-02T00:42:11Z

💔 Test failed - checks-actions

ChrisDenton · 2024-09-02T00:49:21Z

@bors try

bors · 2024-09-02T00:50:32Z

⌛ Trying commit 3b6f0ec with merge f823093...

Move the Windows remove_dir_all impl into a module and make it more race resistant This attempts to make the Windows implementation of `remove_dir_all` easier to understand and work with by separating out different concerns into their own functions. The code is mostly the same as before just moved around. There are some changes to make it more robust against races (e.g. two calls to `remove_dir_all` running concurrently). The module level comment explains the issue.

bors · 2024-09-02T02:44:11Z

☀️ Try build successful - checks-actions
Build commit: f823093 (f823093a836e2e652e64835d792119f822f74de3)

ChrisDenton · 2024-09-02T02:44:24Z

oops, I was trying linux. Let's do that again...

@bors try

Move the Windows remove_dir_all impl into a module and make it more race resistant This attempts to make the Windows implementation of `remove_dir_all` easier to understand and work with by separating out different concerns into their own functions. The code is mostly the same as before just moved around. There are some changes to make it more robust against races (e.g. two calls to `remove_dir_all` running concurrently). The module level comment explains the issue. try-job: x86_64-msvc try-job: i686-msvc

bors · 2024-09-02T02:45:35Z

⌛ Trying commit 3b6f0ec with merge ece733c...

bors · 2024-09-02T04:22:46Z

💔 Test failed - checks-actions

ChrisDenton · 2024-09-02T04:55:21Z

Ok, I'm going to give up on enabling the test for now. Given existing CI issues, I fear it's going to be a bit flaky no matter what.

Amanieu · 2024-09-02T07:20:17Z

library/std/src/sys/pal/windows/fs/remove_dir_all.rs

+fn delete(parent: &File, name: &[u16]) -> Result<(), WinError> {
+    // Note that the `delete` function consumes the opened file to ensure it's
+    // dropped immediately. See module comments for why this is important.
+    match open_link_no_reparse(parent, name, c::SYNCHRONIZE | c::DELETE) {


Can't we just use NtDeleteFile here and avoid having to temporarily open the file? Would this avoid the ERROR_DELETE_PENDING case?

NtDeleteFile has a couple of issues:

the file isn't deleted until every handle to it is closed, system wide

it doesn't delete files that have the readonly attribute set (which, for example, git makes heavy use of)

Amanieu · 2024-09-02T23:13:25Z

@bors r+

bors · 2024-09-02T23:13:27Z

📌 Commit bb9d5c4 has been approved by Amanieu

It is now in the queue for this repository.

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#129152 (custom/external clippy support for bootstrapping) - rust-lang#129311 (don't copy `.rustc-dev-contents` from CI rustc) - rust-lang#129800 (Move the Windows remove_dir_all impl into a module and make it more race resistant) - rust-lang#129860 (update `object` dependency to remove duplicate `wasmparser`) - rust-lang#129885 (chore: remove repetitive words) - rust-lang#129913 (Add missing read_buf stub for x86_64-unknown-l4re-uclibc) - rust-lang#129916 (process.rs: remove "Basic usage" text where not useful) - rust-lang#129917 (Fix parsing of beta version in dry-run mode) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#129800 - ChrisDenton:remove-dir-all2, r=Amanieu Move the Windows remove_dir_all impl into a module and make it more race resistant This attempts to make the Windows implementation of `remove_dir_all` easier to understand and work with by separating out different concerns into their own functions. The code is mostly the same as before just moved around. There are some changes to make it more robust against races (e.g. two calls to `remove_dir_all` running concurrently). The module level comment explains the issue. try-job: x86_64-msvc try-job: i686-msvc

…nieu Win: Open dir for sync access in remove_dir_all A small follow up to rust-lang#129800. We should explicitly open directories for synchronous access. We ultimately use `GetFileInformationByHandleEx` to read directories which should paper over any issues caused by using async directory reads (or else return an error) but it's better to do the right thing in the first place. Note though that `delete` does not read or write any data so it's not necessary there.

Rollup merge of rust-lang#129934 - ChrisDenton:remove-dir-all3, r=Amanieu Win: Open dir for sync access in remove_dir_all A small follow up to rust-lang#129800. We should explicitly open directories for synchronous access. We ultimately use `GetFileInformationByHandleEx` to read directories which should paper over any issues caused by using async directory reads (or else return an error) but it's better to do the right thing in the first place. Note though that `delete` does not read or write any data so it's not necessary there.

rustbot assigned Amanieu Aug 31, 2024

rustbot added O-windows Operating system: Windows S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 31, 2024

This comment has been minimized.

Sign in to view

ChrisDenton force-pushed the remove-dir-all2 branch from 5fa0843 to 2ab3c15 Compare August 31, 2024 03:13

Move remove_dir_all impl into a module

bb9d5c4

ChrisDenton force-pushed the remove-dir-all2 branch from 2ab3c15 to bb9d5c4 Compare August 31, 2024 12:19

rustbot added the A-run-make Area: port run-make Makefiles to rmake.rs label Sep 1, 2024

This comment has been minimized.

Sign in to view

bors 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 Sep 2, 2024

ChrisDenton force-pushed the remove-dir-all2 branch from fca4020 to 3b6f0ec Compare September 2, 2024 00:48

This comment has been minimized.

Sign in to view

ChrisDenton force-pushed the remove-dir-all2 branch from 3b6f0ec to bb9d5c4 Compare September 2, 2024 04:54

Amanieu reviewed Sep 2, 2024

View reviewed changes

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 2, 2024

matthiaskrgr mentioned this pull request Sep 3, 2024

Rollup of 8 pull requests #129922

Merged

bors merged commit 72cc383 into rust-lang:master Sep 3, 2024
12 checks passed

rustbot added this to the 1.83.0 milestone Sep 3, 2024

ChrisDenton mentioned this pull request Sep 3, 2024

Win: Open dir for sync access in remove_dir_all #129934

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move the Windows remove_dir_all impl into a module and make it more race resistant #129800

Move the Windows remove_dir_all impl into a module and make it more race resistant #129800

ChrisDenton commented Aug 31, 2024 •

edited

Loading

This comment has been minimized.

rustbot commented Sep 1, 2024

ChrisDenton commented Sep 1, 2024

bors commented Sep 1, 2024

This comment has been minimized.

bors commented Sep 2, 2024

ChrisDenton commented Sep 2, 2024

bors commented Sep 2, 2024

bors commented Sep 2, 2024

ChrisDenton commented Sep 2, 2024

bors commented Sep 2, 2024

This comment has been minimized.

bors commented Sep 2, 2024

ChrisDenton commented Sep 2, 2024

Amanieu Sep 2, 2024

ChrisDenton Sep 2, 2024

Amanieu commented Sep 2, 2024

bors commented Sep 2, 2024

Move the Windows remove_dir_all impl into a module and make it more race resistant #129800

Move the Windows remove_dir_all impl into a module and make it more race resistant #129800

Conversation

ChrisDenton commented Aug 31, 2024 • edited Loading

This comment has been minimized.

rustbot commented Sep 1, 2024

ChrisDenton commented Sep 1, 2024

bors commented Sep 1, 2024

This comment has been minimized.

bors commented Sep 2, 2024

ChrisDenton commented Sep 2, 2024

bors commented Sep 2, 2024

bors commented Sep 2, 2024

ChrisDenton commented Sep 2, 2024

bors commented Sep 2, 2024

This comment has been minimized.

bors commented Sep 2, 2024

ChrisDenton commented Sep 2, 2024

Amanieu Sep 2, 2024

Choose a reason for hiding this comment

ChrisDenton Sep 2, 2024

Choose a reason for hiding this comment

Amanieu commented Sep 2, 2024

bors commented Sep 2, 2024

ChrisDenton commented Aug 31, 2024 •

edited

Loading