Revert test directory cleaning change.

[ehuss]

#6900 changed it so that the entire cit directory was cleaned once when tests started. Previously, each t# directory was deleted just before each test ran. This restores the old behavior due to problems on Windows.

The problem is that the call to rm_rf would fail with various errors ("Not found", "directory not empty", etc.) if you run cargo test twice. The first panic would poison the lazy static initializer, causing all subsequent tests to fail.

There are a variety of reasons deleting a file on Windows is difficult. My hypothesis in this case is that services like the indexing service and Defender swoop in and temporarily hold handles to files. This seems to be worse on slower systems, where presumably these services take longer to process all the files created by the test suite. It may also be related to how files are "marked for deletion" but are not immediately deleted.

The solution here is to spread out the deletion over time, giving Windows more of an opportunity to release its handles. This is a poor solution, and should only help reduce the frequency, but not entirely fix it.

I believe that this cannot be solved using DeleteFileW. There are more details at rust-lang/rust#29497, which is a long-standing problem that there are no good Rust implementations for recursively deleting a directory.

An example of something that implements a "safe" delete is Cygwin's unlink implementation. As you can see, it is quite complex. Of course our use case does not need to handle quite as many edge cases, but I think any implementation is going to be nontrivial, and require Windows-specific APIs not available in std.

Note: Even before #6900 I still get a lot of errors on a slow VM (particularly "directory not empty"), with Defender and Indexing off. I'm not sure why. This PR should make it more bearable, though.

[rust-highfive]

r? @nrc

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

[Eh2406]

Thank you very much for working on this! I have 2 random thoughts,

1. I know that @rbtcollins has been doing great work to make rustup's IO faster on windows, I wonder if we can steal some ideas work with them to make this faster.
2. I have heard a rumor that there is a programmatic way to turn Indexing off on a per directory bases. If that rumor is true, then we should do that for cit. (If it works well there we should consider doing it for some or all of each profile directory and some or all of ~/cargo)

[alexcrichton]

@bors: r+

[bors]

📌 Commit 0082290 has been approved by alexcrichton

[bors]

⌛ Testing commit 0082290 with merge 2ee292f...

[ehuss]

@Eh2406 It would be nice to have a Windows expert help. There is code to disable backups of target on macos. Better support like that on windows would be good.

[bors]

☀️ Test successful - checks-travis, status-appveyor
Approved by: alexcrichton
Pushing 2ee292f to master...

[rbtcollins]

Hi, Indexing service does indeed honour FILE_ATTRIBUTE_NOT_CONTENT_INDEXED. I'm not sure if it will eliminate handle races from it because the hook will still need to examine the file to determine not to index it... but it will reduce system load, OTOH IndexService self throttles under high load anyway...

Defender or other AV systems also do hold handles on the files at various times; we haven't spotted a Defender handle on windows 10 so far - it seems to do its access all in-line in IOPS from our processes, but Symantec and other scanner seem to actively race.

This approach should be robust - rust-lang/rustup#1873 - its obviously not suitable for a stdlib API call, but should be fine for cargo or other higher layer crates. - in short, spin for some short period. The 28 seconds we use here is because we have long lived proxy processes; for the cargo test suite I'd suggest a spin period of no more than 50ms or so.

If you can reproduce the problem, running under WPA or procmon and getting insight into what process is holding handles would perhaps allow more designs to be proposed for dealing with the issue.

If, for instance, it is defender then in CI jobs, powershell can be used to programmatically disable Defender for a directory; this isn't safe to do for development machines which can be targetted, but for a one-off random directory in a cloud instance its pretty safe.

Grab me on discord in the wg-rustup channel if you want to give me more context, I'm happy to help.

[ehuss]

Last time I investigated this about a year ago (#5481 (comment)), all I was able to determine is that one of the service processes had a handle open, and I have no idea how to tell which service was responsible. It also included the "system" process, which was not helpful.

It doesn't really matter, the rm code should be resilient to this situation. For example, looking at the cygwin implementation, I think when it fails it moves it to the trash bin instead. Perhaps something like that is the solution, though I'm not sure if it's a good solution.

Also, from that same issue (5481) there are some really tricky problems. Deleting an executable can succeed, but leave a phantom behind (such that path.exists() is true immediately after a successful unlink call, and you cannot reuse the same filename for a few milliseconds). Executables (that were recently created and ran) are somehow special, and the cargo test suite creates a lot of them very rapidly.

The jitter code also sounds like a good idea, but I don't really have time to implement it right now.

@Eh2406 can you let me know if you run into problems after this PR? I'll make it a higher priority if it is still bugging you.

[Eh2406]

#7011 means that I am not running into this much these days. I would not prioritize fixing the test sweet.

[rbtcollins]

@ehuss WPA or procmon would give much more detailed diagnostics about handles; your restart manager API technique was clever but too indirect I think.

https://github.com/cygwin/cygwin/blob/ad101bcb0f55f0eb1a9f60187f949c3decd855e4/winsup/cygwin/syscalls.cc#L930 is the rm -r mitigation with https://github.com/cygwin/cygwin/blob/ad101bcb0f55f0eb1a9f60187f949c3decd855e4/winsup/cygwin/syscalls.cc#L636 being the referenced comment. I can't shed much light on those techniques (they post date my active involvement in cygwin.

The moving to the recycle bin is a reasonable approach when direct unlinking doesn't work for test suites, but you would need the full code path - folk will run the test suite on network drives and so forth, where that doesn't work.

My point about about gathering data is that making rm resilient to the situation depends on knowing what actually is happening. If cargo is encountering this well known WTF mode where unlinking a directory returns before the unlink actually happens, then its pretty straight forward to mitigate - that appears to be very robust with the move-then-unlink path in rustup. If it is a handle collision with a system utility (index service / virus scanner etc), then a jitter based retry can mitigate that.

One could of course just through several techniques at it and hope :). Anyhow - my offer is open, let me know if you need help.