> On Nov. 29, 2017, 5:36 p.m., Akash Gupta wrote: > > 3rdparty/stout/include/stout/os/windows/rmdir.hpp > > Lines 43 (patched) > > <https://reviews.apache.org/r/64181/diff/1/?file=1904121#file1904121line45> > > > > I think it makes sense to wrap `RemoveFileW` and `RemoveDirectoryW` > > with this function so that it works similar to linux. Otherwise, another > > part of mesos might hit the same issue if they just call `os::rm` or > > `os::rmdir (not recursive)`
Yeah, I agree. That will give `os::rm` semantics much closer to those of POSIX. - Andrew ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64181/#review192213 ----------------------------------------------------------- On Nov. 29, 2017, 2:45 p.m., Andrew Schwartzmeyer wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64181/ > ----------------------------------------------------------- > > (Updated Nov. 29, 2017, 2:45 p.m.) > > > Review request for mesos, Akash Gupta, John Kordich, and Joseph Wu. > > > Repository: mesos > > > Description > ------- > > The biggest bug with `os::rmdir` was the race condition that appeared on > Windows. There is now method to delete a file on Windows synchronously. > Furthermore, the `RemoveDirectory` API requires that the directory > actually be empty; that is, it does contain files that are marked for > deletion but not yet deleted. Avoiding this race condition requires > waiting for the file to be deleted, not just marked for deletion. > > This was accomplished by simplifying the `recursive_remove_directory` > code so that the base recursion case deletes files and symlinks, and > adding a wait in the depth-first search after each recursion. > > Furthermore, `os::rmdir` was incorrectly calling `os::realpath`. The > specification of `os::rmdir` states that it expects an absolute path. > The error condition is that the path is not a directory. A symlink to a > directory is not a directory, so do not follow semantics are required. > > In the non-recursive case, a stray ANSI CRT function was still in-use, > instead of a long-path aware Unicode Windows API. > > > Diffs > ----- > > 3rdparty/stout/include/stout/internal/windows/longpath.hpp > eb62dd6d4cb726de310a962c07ce5620e2939d17 > 3rdparty/stout/include/stout/os/windows/rmdir.hpp > 76b74f853393c08020d3b713bcd0f9ce12032acd > > > Diff: https://reviews.apache.org/r/64181/diff/1/ > > > Testing > ------- > > --gtest_repeat=100 > > 1 second wait seems to be long enough. > > > Thanks, > > Andrew Schwartzmeyer > >
