You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alex Clemmer <cl...@gmail.com> on 2017/01/08 23:52:49 UTC
Review Request 55327: Windows: Fixed hanging symlink bug in
`os::rmdir`.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55327/
-----------------------------------------------------------
Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
Repository: mesos
Description
-------
The Windows implementation of `os::rmdir` will fail to delete "hanging"
symlinks (i.e., symlinks whose targets do not exist). Note that on
Windows this bug is specific to symlinks whose targets are _deleted_,
since it is impossible to create a symlink whose target does not exist.
The primary issue that causes this problem is that it is very difficult
to tell whether a symlink points at a directory or a file unless you
resolve the symlink and determine whether the target is a directory or a
file. In situations where the target does not exist, we can't use this
information, and so `os::rmdir` occasionally mis-routes a symlink to
(what was) a directory to a `::remove` call, which will fail with a
cryptic error.
To fix this behavior, this commit will introduce code that simply tries
to remove the reparse point with both `RemoveDirectory` and
`DeleteFile`, and if either succeeds, we report success for the
operation. This represents a "best effort"; in the case that the reparse
point represents something more exotic than a symlink, we will still
fail, but by choosing not to verify whether the target is a directory or
a file, we simplify the code and still obtain the outcome of having
deleted the directory.
This commit is the primary blocker for MESOS-6707, as deleting the Agent
sandbox will sometimes cause us to delete the latest run directory for
the executor before the symlinked `latest` directory itself. This causes
the delete to fail, and then the GC tests to fail, since they tend to
assert the directory does not exist.
Diffs
-----
3rdparty/stout/include/stout/os/windows/rmdir.hpp 4437484c068e9ef046e0be14683c97db447f2da1
3rdparty/stout/tests/os/rmdir_tests.cpp 988d41b7fdd11cc96ce005671a7c62d1b5a3615d
Diff: https://reviews.apache.org/r/55327/diff/
Testing
-------
Thanks,
Alex Clemmer
Re: Review Request 55327: Windows: Fixed hanging symlink bug in
`os::rmdir`.
Posted by Alex Clemmer <cl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55327/
-----------------------------------------------------------
(Updated Jan. 18, 2017, 8:50 p.m.)
Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
Changes
-------
Address Joseph's comments, fix embarrasing mistake.
Repository: mesos
Description
-------
The Windows implementation of `os::rmdir` will fail to delete "hanging"
symlinks (i.e., symlinks whose targets do not exist). Note that on
Windows this bug is specific to symlinks whose targets are _deleted_,
since it is impossible to create a symlink whose target does not exist.
The primary issue that causes this problem is that it is very difficult
to tell whether a symlink points at a directory or a file unless you
resolve the symlink and determine whether the target is a directory or a
file. In situations where the target does not exist, we can't use this
information, and so `os::rmdir` occasionally mis-routes a symlink to
(what was) a directory to a `::remove` call, which will fail with a
cryptic error.
To fix this behavior, this commit will introduce code that simply tries
to remove the reparse point with both `RemoveDirectory` and
`DeleteFile`, and if either succeeds, we report success for the
operation. This represents a "best effort"; in the case that the reparse
point represents something more exotic than a symlink, we will still
fail, but by choosing not to verify whether the target is a directory or
a file, we simplify the code and still obtain the outcome of having
deleted the directory.
This commit is the primary blocker for MESOS-6707, as deleting the Agent
sandbox will sometimes cause us to delete the latest run directory for
the executor before the symlinked `latest` directory itself. This causes
the delete to fail, and then the GC tests to fail, since they tend to
assert the directory does not exist.
Diffs (updated)
-----
3rdparty/stout/include/stout/os/windows/rmdir.hpp 4437484c068e9ef046e0be14683c97db447f2da1
3rdparty/stout/tests/os/rmdir_tests.cpp 988d41b7fdd11cc96ce005671a7c62d1b5a3615d
Diff: https://reviews.apache.org/r/55327/diff/
Testing
-------
Thanks,
Alex Clemmer
Re: Review Request 55327: Windows: Fixed hanging symlink bug in
`os::rmdir`.
Posted by Alex Clemmer <cl...@gmail.com>.
> On Jan. 18, 2017, 2:32 a.m., Joseph Wu wrote:
> > 3rdparty/stout/include/stout/os/windows/rmdir.hpp, lines 120-123
> > <https://reviews.apache.org/r/55327/diff/2/?file=1605538#file1605538line120>
> >
> > Note: This is actually unreachable.
:| Embarrassing.
- Alex
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55327/#review162019
-----------------------------------------------------------
On Jan. 15, 2017, 8:40 a.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55327/
> -----------------------------------------------------------
>
> (Updated Jan. 15, 2017, 8:40 a.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The Windows implementation of `os::rmdir` will fail to delete "hanging"
> symlinks (i.e., symlinks whose targets do not exist). Note that on
> Windows this bug is specific to symlinks whose targets are _deleted_,
> since it is impossible to create a symlink whose target does not exist.
>
> The primary issue that causes this problem is that it is very difficult
> to tell whether a symlink points at a directory or a file unless you
> resolve the symlink and determine whether the target is a directory or a
> file. In situations where the target does not exist, we can't use this
> information, and so `os::rmdir` occasionally mis-routes a symlink to
> (what was) a directory to a `::remove` call, which will fail with a
> cryptic error.
>
> To fix this behavior, this commit will introduce code that simply tries
> to remove the reparse point with both `RemoveDirectory` and
> `DeleteFile`, and if either succeeds, we report success for the
> operation. This represents a "best effort"; in the case that the reparse
> point represents something more exotic than a symlink, we will still
> fail, but by choosing not to verify whether the target is a directory or
> a file, we simplify the code and still obtain the outcome of having
> deleted the directory.
>
> This commit is the primary blocker for MESOS-6707, as deleting the Agent
> sandbox will sometimes cause us to delete the latest run directory for
> the executor before the symlinked `latest` directory itself. This causes
> the delete to fail, and then the GC tests to fail, since they tend to
> assert the directory does not exist.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/os/windows/rmdir.hpp 4437484c068e9ef046e0be14683c97db447f2da1
> 3rdparty/stout/tests/os/rmdir_tests.cpp 988d41b7fdd11cc96ce005671a7c62d1b5a3615d
>
> Diff: https://reviews.apache.org/r/55327/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>
Re: Review Request 55327: Windows: Fixed hanging symlink bug in
`os::rmdir`.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55327/#review162019
-----------------------------------------------------------
Ship it!
3rdparty/stout/include/stout/os/windows/rmdir.hpp (line 95)
<https://reviews.apache.org/r/55327/#comment233281>
Change the order of this comment:
```
whether it's a symlink, directory, or file.
```
3rdparty/stout/include/stout/os/windows/rmdir.hpp (lines 105 - 123)
<https://reviews.apache.org/r/55327/#comment233283>
This may be clearer like:
```
const BOOL rmdir = ::RemoveDirectory(current_absolute_path.c_str());
if (rmdir == FALSE) {
const BOOL rm = ::DeleteFile(current_absolute_path.c_str());
if (rm == FALSE) {
return WindowsError(
"Failed to remove symlink '" + current_absolute_path + "'");
}
}
```
3rdparty/stout/include/stout/os/windows/rmdir.hpp (lines 120 - 123)
<https://reviews.apache.org/r/55327/#comment233284>
Note: This is actually unreachable.
3rdparty/stout/include/stout/os/windows/rmdir.hpp (line 137)
<https://reviews.apache.org/r/55327/#comment233282>
Remove this note.
- Joseph Wu
On Jan. 15, 2017, 12:40 a.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55327/
> -----------------------------------------------------------
>
> (Updated Jan. 15, 2017, 12:40 a.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The Windows implementation of `os::rmdir` will fail to delete "hanging"
> symlinks (i.e., symlinks whose targets do not exist). Note that on
> Windows this bug is specific to symlinks whose targets are _deleted_,
> since it is impossible to create a symlink whose target does not exist.
>
> The primary issue that causes this problem is that it is very difficult
> to tell whether a symlink points at a directory or a file unless you
> resolve the symlink and determine whether the target is a directory or a
> file. In situations where the target does not exist, we can't use this
> information, and so `os::rmdir` occasionally mis-routes a symlink to
> (what was) a directory to a `::remove` call, which will fail with a
> cryptic error.
>
> To fix this behavior, this commit will introduce code that simply tries
> to remove the reparse point with both `RemoveDirectory` and
> `DeleteFile`, and if either succeeds, we report success for the
> operation. This represents a "best effort"; in the case that the reparse
> point represents something more exotic than a symlink, we will still
> fail, but by choosing not to verify whether the target is a directory or
> a file, we simplify the code and still obtain the outcome of having
> deleted the directory.
>
> This commit is the primary blocker for MESOS-6707, as deleting the Agent
> sandbox will sometimes cause us to delete the latest run directory for
> the executor before the symlinked `latest` directory itself. This causes
> the delete to fail, and then the GC tests to fail, since they tend to
> assert the directory does not exist.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/os/windows/rmdir.hpp 4437484c068e9ef046e0be14683c97db447f2da1
> 3rdparty/stout/tests/os/rmdir_tests.cpp 988d41b7fdd11cc96ce005671a7c62d1b5a3615d
>
> Diff: https://reviews.apache.org/r/55327/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>
Re: Review Request 55327: Windows: Fixed hanging symlink bug in
`os::rmdir`.
Posted by Alex Clemmer <cl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55327/
-----------------------------------------------------------
(Updated Jan. 15, 2017, 8:40 a.m.)
Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
Changes
-------
Fix misspelling.
Repository: mesos
Description
-------
The Windows implementation of `os::rmdir` will fail to delete "hanging"
symlinks (i.e., symlinks whose targets do not exist). Note that on
Windows this bug is specific to symlinks whose targets are _deleted_,
since it is impossible to create a symlink whose target does not exist.
The primary issue that causes this problem is that it is very difficult
to tell whether a symlink points at a directory or a file unless you
resolve the symlink and determine whether the target is a directory or a
file. In situations where the target does not exist, we can't use this
information, and so `os::rmdir` occasionally mis-routes a symlink to
(what was) a directory to a `::remove` call, which will fail with a
cryptic error.
To fix this behavior, this commit will introduce code that simply tries
to remove the reparse point with both `RemoveDirectory` and
`DeleteFile`, and if either succeeds, we report success for the
operation. This represents a "best effort"; in the case that the reparse
point represents something more exotic than a symlink, we will still
fail, but by choosing not to verify whether the target is a directory or
a file, we simplify the code and still obtain the outcome of having
deleted the directory.
This commit is the primary blocker for MESOS-6707, as deleting the Agent
sandbox will sometimes cause us to delete the latest run directory for
the executor before the symlinked `latest` directory itself. This causes
the delete to fail, and then the GC tests to fail, since they tend to
assert the directory does not exist.
Diffs (updated)
-----
3rdparty/stout/include/stout/os/windows/rmdir.hpp 4437484c068e9ef046e0be14683c97db447f2da1
3rdparty/stout/tests/os/rmdir_tests.cpp 988d41b7fdd11cc96ce005671a7c62d1b5a3615d
Diff: https://reviews.apache.org/r/55327/diff/
Testing
-------
Thanks,
Alex Clemmer
Re: Review Request 55327: Windows: Fixed hanging symlink bug in
`os::rmdir`.
Posted by Lior Zeno <li...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55327/#review160930
-----------------------------------------------------------
3rdparty/stout/include/stout/os/windows/rmdir.hpp (line 97)
<https://reviews.apache.org/r/55327/#comment232165>
s/best-effor/best-effort
- Lior Zeno
On Jan. 8, 2017, 11:52 p.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55327/
> -----------------------------------------------------------
>
> (Updated Jan. 8, 2017, 11:52 p.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The Windows implementation of `os::rmdir` will fail to delete "hanging"
> symlinks (i.e., symlinks whose targets do not exist). Note that on
> Windows this bug is specific to symlinks whose targets are _deleted_,
> since it is impossible to create a symlink whose target does not exist.
>
> The primary issue that causes this problem is that it is very difficult
> to tell whether a symlink points at a directory or a file unless you
> resolve the symlink and determine whether the target is a directory or a
> file. In situations where the target does not exist, we can't use this
> information, and so `os::rmdir` occasionally mis-routes a symlink to
> (what was) a directory to a `::remove` call, which will fail with a
> cryptic error.
>
> To fix this behavior, this commit will introduce code that simply tries
> to remove the reparse point with both `RemoveDirectory` and
> `DeleteFile`, and if either succeeds, we report success for the
> operation. This represents a "best effort"; in the case that the reparse
> point represents something more exotic than a symlink, we will still
> fail, but by choosing not to verify whether the target is a directory or
> a file, we simplify the code and still obtain the outcome of having
> deleted the directory.
>
> This commit is the primary blocker for MESOS-6707, as deleting the Agent
> sandbox will sometimes cause us to delete the latest run directory for
> the executor before the symlinked `latest` directory itself. This causes
> the delete to fail, and then the GC tests to fail, since they tend to
> assert the directory does not exist.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/os/windows/rmdir.hpp 4437484c068e9ef046e0be14683c97db447f2da1
> 3rdparty/stout/tests/os/rmdir_tests.cpp 988d41b7fdd11cc96ce005671a7c62d1b5a3615d
>
> Diff: https://reviews.apache.org/r/55327/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>