Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2663183003:
Make sure tests delete temp files on Windows (Closed)
|
Created:
3 years, 10 months ago by Tomasz Moniuszko Modified:
3 years, 10 months ago CC:
chromium-reviews, vmpstr+watch_chromium.org, phajdan, grt (UTC plus 2) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake sure tests delete temp files on Windows
BUG=671990
Patch Set 1 #
Total comments: 9
Messages
Total messages: 10 (3 generated)
Description was changed from ========== Make sure tests delete temp files on Windows BUG= ========== to ========== Make sure tests delete temp files on Windows BUG=671990 ==========
tmoniuszko@opera.com changed reviewers: + phajdan.jr@chromium.org
phajdan.jr@ With this CL applied base::DeleteFile() no longer fails when test process is removing temporary files created for child processes. I think this fix is related to bug 671990. Bug description suggests a change to the handles inherited by child process. I found that it's enough to add FILE_SHARE_DELETE flag when file is being opened for reading.
https://codereview.chromium.org/2663183003/diff/1/base/test/launcher/test_lau... File base/test/launcher/test_launcher.cc (right): https://codereview.chromium.org/2663183003/diff/1/base/test/launcher/test_lau... base/test/launcher/test_launcher.cc:60: #include <limits> nit: Include system headers before chromium headers (except the first one of course), i.e. right before <memory>. Probably no need to keep that windows-specific. https://codereview.chromium.org/2663183003/diff/1/base/test/launcher/test_lau... base/test/launcher/test_launcher.cc:175: // Does the same as base::ReadFileToString() but uses FILE_SHARE_DELETE flag I'm worried about duplication here. Actually from the bug, it seems a better solution would be the following: "The launcher should use base::Launch's |handles_to_inherit| so rather than |inherit_handles| so that each child only inherits its one output handle." Let me know if you'd be interested in writing a patch to that effect.
https://codereview.chromium.org/2663183003/diff/1/base/test/launcher/test_lau... File base/test/launcher/test_launcher.cc (right): https://codereview.chromium.org/2663183003/diff/1/base/test/launcher/test_lau... base/test/launcher/test_launcher.cc:363: since the problem is that new_options.stdout_handle is inherited into every subproc, another approach is to revoke that handle's ability to be inherited here just after the correct subprocess was launched. this is a bit hacky, but something like: if (new_options.stdout_handle) { // Prevent leakage of this handle into other subprocs. ::SetHandleInformation(new_options.stdout_handle, HANDLE_FLAG_INHERIT, 0); }
https://codereview.chromium.org/2663183003/diff/1/base/test/launcher/test_lau... File base/test/launcher/test_launcher.cc (right): https://codereview.chromium.org/2663183003/diff/1/base/test/launcher/test_lau... base/test/launcher/test_launcher.cc:175: // Does the same as base::ReadFileToString() but uses FILE_SHARE_DELETE flag On 2017/02/01 11:10:48, Paweł Hajdan Jr. wrote: > I'm worried about duplication here. > > Actually from the bug, it seems a better solution would be the following: > > "The launcher should use base::Launch's |handles_to_inherit| so rather than > |inherit_handles| so that each child only inherits its one output handle." > > Let me know if you'd be interested in writing a patch to that effect. I tried to add the handle to temp file to |handles_to_inherit| vector. With this change temp files still persist. Additionally setting options.inherit_handles=false seem to solve the temp files problem (although I haven't tested it intensively) but prevents parent process from printing details about test failures - just like in case of clearing HANDLE_FLAG_INHERIT for handle (see my other comment). Also, I wonder how this issue is related to child processes. My changes in this CL don't change the way of passing handle to child process. It changes the way how parent process opens the output file for reading after child process is finished. ReadFileToString() opens file without FILE_SHARE_DELETE flag. It closes the handle before calling DeleteFile() so everything should be just fine. For some reason it's not. Maybe it's somehow related to system caching if FILE_ATTRIBUTE_TEMPORARY is used: https://msdn.microsoft.com/en-us/library/windows/desktop/aa363858(v=vs.85).as... Adding FILE_SHARE_DELETE flag solves this problem. https://codereview.chromium.org/2663183003/diff/1/base/test/launcher/test_lau... base/test/launcher/test_launcher.cc:363: On 2017/02/01 13:04:44, grt (UTC plus 1) wrote: > since the problem is that new_options.stdout_handle is inherited into every > subproc, another approach is to revoke that handle's ability to be inherited > here just after the correct subprocess was launched. this is a bit hacky, but > something like: > > if (new_options.stdout_handle) { > // Prevent leakage of this handle into other subprocs. > ::SetHandleInformation(new_options.stdout_handle, HANDLE_FLAG_INHERIT, > 0); > } It doesn't seem to work. I added the above code to LaunchChildTestProcessWithOptions(). I'm still getting "Failed to delete" warning from DoLaunchChildTestProcess(). Some temp files still stay in temp folder when test is finished. Also test process no longer prints information about failed test (for example when one of EXPECT_ gtest expectations fails). Parent process just prints names of failed tests in the summary.
grt@chromium.org changed reviewers: + grt@chromium.org
The behavior I noticed when I filed the bug was that these tmp files were being leaked on account of them not being deletable. The reason they couldn't be deleted in parent P after child A had exited is that child B, C, D... all inherited the same handle, therefore holding the file open. There is no valid reason for the other children to have this handle, so I think the correct fix is to be sure that handles intended for child A are not inherited by other child procs. https://codereview.chromium.org/2663183003/diff/1/base/test/launcher/test_lau... File base/test/launcher/test_launcher.cc (right): https://codereview.chromium.org/2663183003/diff/1/base/test/launcher/test_lau... base/test/launcher/test_launcher.cc:175: // Does the same as base::ReadFileToString() but uses FILE_SHARE_DELETE flag On 2017/02/01 11:10:48, Paweł Hajdan Jr. wrote: > I'm worried about duplication here. > > Actually from the bug, it seems a better solution would be the following: > > "The launcher should use base::Launch's |handles_to_inherit| so rather than > |inherit_handles| so that each child only inherits its one output handle." I wrote that in the bug before realizing that specifying handles to inherit isn't compatible with the current approach for specifying std handles. > Let me know if you'd be interested in writing a patch to that effect. https://codereview.chromium.org/2663183003/diff/1/base/test/launcher/test_lau... base/test/launcher/test_launcher.cc:363: On 2017/02/02 16:16:43, Tomasz Moniuszko wrote: > On 2017/02/01 13:04:44, grt (UTC plus 1) wrote: > > since the problem is that new_options.stdout_handle is inherited into every > > subproc, another approach is to revoke that handle's ability to be inherited > > here just after the correct subprocess was launched. this is a bit hacky, but > > something like: > > > > if (new_options.stdout_handle) { > > // Prevent leakage of this handle into other subprocs. > > ::SetHandleInformation(new_options.stdout_handle, HANDLE_FLAG_INHERIT, > > 0); > > } > > It doesn't seem to work. Did you try it without your other changes? I just tried locally and it seemed to work as desired. > I added the above code to > LaunchChildTestProcessWithOptions(). I'm still getting "Failed to delete" > warning from DoLaunchChildTestProcess(). Some temp files still stay in temp > folder when test is finished. Also test process no longer prints information > about failed test (for example when one of EXPECT_ gtest expectations fails). > Parent process just prints names of failed tests in the summary.
https://codereview.chromium.org/2663183003/diff/1/base/test/launcher/test_lau... File base/test/launcher/test_launcher.cc (right): https://codereview.chromium.org/2663183003/diff/1/base/test/launcher/test_lau... base/test/launcher/test_launcher.cc:363: On 2017/02/03 09:53:23, grt (UTC plus 1) wrote: > On 2017/02/02 16:16:43, Tomasz Moniuszko wrote: > > On 2017/02/01 13:04:44, grt (UTC plus 1) wrote: > > > since the problem is that new_options.stdout_handle is inherited into every > > > subproc, another approach is to revoke that handle's ability to be inherited > > > here just after the correct subprocess was launched. this is a bit hacky, > but > > > something like: > > > > > > if (new_options.stdout_handle) { > > > // Prevent leakage of this handle into other subprocs. > > > ::SetHandleInformation(new_options.stdout_handle, HANDLE_FLAG_INHERIT, > > > 0); > > > } > > > > It doesn't seem to work. > > Did you try it without your other changes? I just tried locally and it seemed to > work as desired. > > > I added the above code to > > LaunchChildTestProcessWithOptions(). I'm still getting "Failed to delete" > > warning from DoLaunchChildTestProcess(). Some temp files still stay in temp > > folder when test is finished. Also test process no longer prints information > > about failed test (for example when one of EXPECT_ gtest expectations fails). > > Parent process just prints names of failed tests in the summary. > Yes, I did it on clean master. I used gpu_unittests for testing (I noticed that these tests leave temp files almost every run). Maybe I put the above code in the wrong place. Your fix is indeed much easier than the solution I uploaded. I will close this review if your fix solves the issue.
https://codereview.chromium.org/2663183003/diff/1/base/test/launcher/test_lau... File base/test/launcher/test_launcher.cc (right): https://codereview.chromium.org/2663183003/diff/1/base/test/launcher/test_lau... base/test/launcher/test_launcher.cc:363: On 2017/02/03 10:08:48, Tomasz Moniuszko wrote: > On 2017/02/03 09:53:23, grt (UTC plus 1) wrote: > > On 2017/02/02 16:16:43, Tomasz Moniuszko wrote: > > > On 2017/02/01 13:04:44, grt (UTC plus 1) wrote: > > > > since the problem is that new_options.stdout_handle is inherited into > every > > > > subproc, another approach is to revoke that handle's ability to be > inherited > > > > here just after the correct subprocess was launched. this is a bit hacky, > > but > > > > something like: > > > > > > > > if (new_options.stdout_handle) { > > > > // Prevent leakage of this handle into other subprocs. > > > > ::SetHandleInformation(new_options.stdout_handle, > HANDLE_FLAG_INHERIT, > > > > 0); > > > > } > > > > > > It doesn't seem to work. > > > > Did you try it without your other changes? I just tried locally and it seemed > to > > work as desired. > > > > > I added the above code to > > > LaunchChildTestProcessWithOptions(). I'm still getting "Failed to delete" > > > warning from DoLaunchChildTestProcess(). Some temp files still stay in temp > > > folder when test is finished. Also test process no longer prints information > > > about failed test (for example when one of EXPECT_ gtest expectations > fails). > > > Parent process just prints names of failed tests in the summary. > > > > Yes, I did it on clean master. I used gpu_unittests for testing (I noticed that > these tests leave temp files almost every run). > > Maybe I put the above code in the wrong place. Your fix is indeed much easier > than the solution I uploaded. I will close this review if your fix solves the > issue. This almost fixes it: https://codereview.chromium.org/2670213004/. See the CL description for a taste of how tricky this is. I'll send up another CL to fix base::OpenFile. Cheers. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
