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.

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

testing: add Windows workarounds to testing.(*T).TempDir's cleanup #44919

Closed
ainar-g opened this issue Mar 10, 2021 · 10 comments
Closed

testing: add Windows workarounds to testing.(*T).TempDir's cleanup #44919

ainar-g opened this issue Mar 10, 2021 · 10 comments

Comments

@ainar-g
Copy link
Contributor

@ainar-g ainar-g commented Mar 10, 2021

In #30789 (CL 172337) a workaround was added to the go command to help with the access denied and similar errors when running tests on Windows. The errors are thought to arise because of antivirus software and similar programs interfering. Currently, similar errors can still happen when using testing.(*T).TempDir during its cleanup stage:

testing.go:915: TempDir RemoveAll cleanup: remove C:\Users\RUNNER~1\AppData\Local\Temp\TestQLogFile_SeekTS_bad010877688\002\812736234.txt: The process cannot access the file because it is being used by another process.

I think that a similar workaround should be applied there. Applying it manually seems to fix the issue.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 10, 2021

It doesn't make sense for us to add a retry loop around every call to os.RemoveAll. The call in cmd/go is arguably a special case: we know that it holds executables that were just run. There is no particular reason for us to think that the files in a testing temporary directory are executables, or are being used by some other process.

Perhaps on Windows if we get a particular error during os.RemoveAll we should sleep briefly and retry. I don't know. But we definitely shouldn't sleep and retry in the testing package.

@ainar-g
Copy link
Contributor Author

@ainar-g ainar-g commented Mar 11, 2021

@ianlancetaylor, thanks for the quick response.

The call in cmd/go is arguably a special case: we know that it holds executables that were just run. There is no particular reason for us to think that the files in a testing temporary directory are executables, or are being used by some other process.

Is it really unreasonable to assume that some people will actually put data that triggers defensive software into the temporary directory? The reason why I filed this issue was actually the fact that I witnessed such failures in tests that only put text (JSON, etc) files there, here on GitHub.

Perhaps on Windows if we get a particular error during os.RemoveAll we should sleep briefly and retry. I don't know. But we definitely shouldn't sleep and retry in the testing package.

That might do it (and would also probably remove the workaround in cmd/go and some other places), but could cause issues down the line if there is no way to disable that behaviour. Just looking at issues from the last few months I was able to find a few similar ones: #38490, #42224, #44782. So the problem is real, and making everyone wrap every call to os.RemoveAll into a loop doesn't sound that appealing.


Alternatively, we could consider this purely an issue with Windows and Windows Defender and propose the solution of “just disable it”. Currently, I don't have enough information on how easily that could be done in a CI/remote server environment for such cases, as I am not a Windows user myself.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 11, 2021

Is it really unreasonable to assume that some people will actually put data that triggers defensive software into the temporary directory?

It's not unreasonable. But the same applies to every directory everywhere. Why should we add a special case to the testing package? What is special there? I explained what is arguably special about the use in cmd/go, but I don't think that argument applies to the testing package.

@ainar-g
Copy link
Contributor Author

@ainar-g ainar-g commented Mar 12, 2021

@ianlancetaylor

I would argue that the special things here are:

  1. testing.(*T).TempDir is a part of the testing API, so the assumption of an average developer would be that it should work “out of the box” on most systems, since people want to test their programs in many different environments. In fact, if I am not mistaken, the cmd/go solution in issue 30789 was introduced precisely to make the cleanup after running tests more robust.
  2. The removal is done in a testing.(*T).Cleanup callback, meaning that developers can't handle the error themselves.
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 15, 2021

I think these arguments in effect apply to every use of os.RemoveAll in the standard library.

And I don't really see why they don't apply to every use of os.RemoveAll everywhere.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 15, 2021

@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Mar 21, 2021

@ainar-g what exactly do you propose to do? I understand you want to retry os.RemoveAll in testing.(*T).Cleanup if it fails. How will you determine the failure? Any error or some particular error? How many times do you propose we retry? How long do we wait before retries? What do we do if we os.RemoveAll still fails after all retries? You need to consider that every PC is different - some systems are slow, others run more "bad" software.

I never see these errors on my computer. But I always disable anti-virus programs (and similar programs) while I develop. These programs slow my computer considerably.

If developers / companies are serious about running antivirus software or any other software that monitors their files, they just need to report these problems to antivirus developers.

I don't see how your proposal can be implemented in os.RemoveAll itself. I think we want os.RemoveAll to return whatever error it receives from OS.

Alex

@ainar-g
Copy link
Contributor Author

@ainar-g ainar-g commented Mar 24, 2021

Sorry, I'm a bit busy with work currently. I'll try to get some better information about the retriable kinds of errors and how I see it implemented within the next few days.

@ainar-g
Copy link
Contributor Author

@ainar-g ainar-g commented Mar 29, 2021

After more digging, it turns out that the tests in the code were bad, and I also feel bad for wasting your time. Apologies!

@ainar-g ainar-g closed this Mar 29, 2021
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 29, 2021

Thanks for following up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants