mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Schwartzmeyer <and...@schwartzmeyer.com>
Subject Re: Review Request 64181: Windows: Fixed `os::rmdir` bugs.
Date Thu, 30 Nov 2017 02:08:38 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message