You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Andrew Schwartzmeyer <an...@schwartzmeyer.com> on 2018/02/02 00:17:28 UTC
Review Request 65467: Windows: Added
`internal::windows::enable_inherit(WindowsFD)`.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65467/
-----------------------------------------------------------
Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu.
Repository: mesos
Description
-------
This function is used on Windows to explicitly enable inheritance on a
handle.
Diffs
-----
3rdparty/stout/include/stout/internal/windows/inherit.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/65467/diff/1/
Testing
-------
Thanks,
Andrew Schwartzmeyer
Re: Review Request 65467: Windows: Added
`internal::windows::set_inherit(WindowsFD, bool)`.
Posted by Jeff Coffler <je...@taltos.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65467/#review197796
-----------------------------------------------------------
Ship it!
Ship It!
- Jeff Coffler
On Feb. 8, 2018, 10:35 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65467/
> -----------------------------------------------------------
>
> (Updated Feb. 8, 2018, 10:35 p.m.)
>
>
> Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu.
>
>
> Bugs: MESOS-8512
> https://issues.apache.org/jira/browse/MESOS-8512
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This function is used on Windows to explicitly enable or disable
> inheritance on a handle.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/internal/windows/inherit.hpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/65467/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 65467: Windows: Added
`internal::windows::set_inherit(WindowsFD, bool)`.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65467/#review197193
-----------------------------------------------------------
Ship it!
Ship It!
- Joseph Wu
On Feb. 8, 2018, 2:35 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65467/
> -----------------------------------------------------------
>
> (Updated Feb. 8, 2018, 2:35 p.m.)
>
>
> Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu.
>
>
> Bugs: MESOS-8512
> https://issues.apache.org/jira/browse/MESOS-8512
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This function is used on Windows to explicitly enable or disable
> inheritance on a handle.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/internal/windows/inherit.hpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/65467/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 65467: Windows: Added
`internal::windows::set_inherit(WindowsFD, bool)`.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65467/
-----------------------------------------------------------
(Updated Feb. 8, 2018, 2:35 p.m.)
Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu.
Changes
-------
Re-arrange commits.
Bugs: MESOS-8512
https://issues.apache.org/jira/browse/MESOS-8512
Repository: mesos
Description
-------
This function is used on Windows to explicitly enable or disable
inheritance on a handle.
Diffs
-----
3rdparty/stout/include/stout/internal/windows/inherit.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/65467/diff/2/
Testing
-------
Thanks,
Andrew Schwartzmeyer
Re: Review Request 65467: Windows: Added
`internal::windows::set_inherit(WindowsFD, bool)`.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65467/
-----------------------------------------------------------
(Updated Feb. 8, 2018, 11:49 a.m.)
Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu.
Changes
-------
Make it `set(bool)` so it can be used to turn things on and off.
Summary (updated)
-----------------
Windows: Added `internal::windows::set_inherit(WindowsFD, bool)`.
Bugs: MESOS-8512
https://issues.apache.org/jira/browse/MESOS-8512
Repository: mesos
Description (updated)
-------
This function is used on Windows to explicitly enable or disable
inheritance on a handle.
Diffs (updated)
-----
3rdparty/stout/include/stout/internal/windows/inherit.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/65467/diff/2/
Changes: https://reviews.apache.org/r/65467/diff/1-2/
Testing
-------
Thanks,
Andrew Schwartzmeyer
Re: Review Request 65467: Windows: Added
`internal::windows::enable_inherit(WindowsFD)`.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On Feb. 6, 2018, 1:55 p.m., Joseph Wu wrote:
> > 3rdparty/stout/include/stout/internal/windows/inherit.hpp
> > Lines 31-43 (patched)
> > <https://reviews.apache.org/r/65467/diff/1/?file=1951450#file1951450line31>
> >
> > This is basically what the ChildHook `UNSET_CLOEXEC` (libprocess/src/subprocess.cpp) should be doing.
>
> Andrew Schwartzmeyer wrote:
> Pretty much, but we don't have any child hook support on Windows whatsoever. Adding support for child hooks would be a non-trivial undertaking (we don't have a pseudo-program written to run child hooks and launch a process like on Linux, we're just launching the process directly).
>
> Joseph Wu wrote:
> Hum... on second though, there appears to be no equivalent for a `UNSET_CLOEXEC` ChildHook on Windows.
>
>
> (This comment also applies to the following review: https://reviews.apache.org/r/65469/ )
>
> We still want to minimize the time each FD spends in an inheritable state, in order to minimize leaks to other forks/processes in other threads. Ideally, any calls to this method should be made right before calls to `subprocess`. The Linux code does this implicitly, by __not__ cloexec-ing certain FDs in some methods.
>
> Correct me if I'm wrong, but FDs do not need to be inheritable in order for `CreateProcessW` to use them as stdout/err FDs.
Correcting: The handles _must_ be inheritable for `CreateProcessW` to use them as standard handles.
> STARTF_USESTDHANDLES: If this flag is specified when calling one of the process creation functions, the handles must be inheritable and the function's bInheritHandles parameter must be set to TRUE.
To redirect the stdin/stdout/stderr of a process made by `CreateProcessW`, the procedure is to set that flag in the `STARTUPINFO`, and then pass the handles via `hStdInput`, `hStdOutput`, and `hStdError`.
I agree that we want to minimize the time a handle is inheritable; so I don't think this is the best solution yet. I think we would want to set the handles as inheritable in the `create_process` wrapper itself, so they're marked inheritable only if they exist and are being passed to a child process. The plus to this is that they will always be inheritable if they are meant to be inherited. The minus to this is that the programmer may not be aware their handle is about to modified and made inheritable (though it should be fairly obvious as it is being passed to a child process). We could then call `CreateProcess` and mark the handles uninheritable after the process has been fully initialized. Tricky part, however, will be avoiding a race, as "the function (`CreateProcess`) returns before the process has finished initialization. The calling thread can use the `WaitForInputIdle` function to wait until the new process has finished its initialization and is waiting for user input with no
input pending." So it is doable.
Additionally, if we do this, we'll want to make sure `os::dup` does not create inheritable handles (and also fix the locations where `::dup` is used).
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65467/#review196932
-----------------------------------------------------------
On Feb. 2, 2018, 12:13 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65467/
> -----------------------------------------------------------
>
> (Updated Feb. 2, 2018, 12:13 p.m.)
>
>
> Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu.
>
>
> Bugs: MESOS-8512
> https://issues.apache.org/jira/browse/MESOS-8512
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This function is used on Windows to explicitly enable inheritance on a
> handle.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/internal/windows/inherit.hpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/65467/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 65467: Windows: Added
`internal::windows::enable_inherit(WindowsFD)`.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On Feb. 6, 2018, 1:55 p.m., Joseph Wu wrote:
> > 3rdparty/stout/include/stout/internal/windows/inherit.hpp
> > Lines 31-43 (patched)
> > <https://reviews.apache.org/r/65467/diff/1/?file=1951450#file1951450line31>
> >
> > This is basically what the ChildHook `UNSET_CLOEXEC` (libprocess/src/subprocess.cpp) should be doing.
Pretty much, but we don't have any child hook support on Windows whatsoever. Adding support for child hooks would be a non-trivial undertaking (we don't have a pseudo-program written to run child hooks and launch a process like on Linux, we're just launching the process directly).
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65467/#review196932
-----------------------------------------------------------
On Feb. 2, 2018, 12:13 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65467/
> -----------------------------------------------------------
>
> (Updated Feb. 2, 2018, 12:13 p.m.)
>
>
> Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu.
>
>
> Bugs: MESOS-8512
> https://issues.apache.org/jira/browse/MESOS-8512
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This function is used on Windows to explicitly enable inheritance on a
> handle.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/internal/windows/inherit.hpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/65467/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 65467: Windows: Added
`internal::windows::enable_inherit(WindowsFD)`.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Feb. 6, 2018, 1:55 p.m., Joseph Wu wrote:
> > 3rdparty/stout/include/stout/internal/windows/inherit.hpp
> > Lines 31-43 (patched)
> > <https://reviews.apache.org/r/65467/diff/1/?file=1951450#file1951450line31>
> >
> > This is basically what the ChildHook `UNSET_CLOEXEC` (libprocess/src/subprocess.cpp) should be doing.
>
> Andrew Schwartzmeyer wrote:
> Pretty much, but we don't have any child hook support on Windows whatsoever. Adding support for child hooks would be a non-trivial undertaking (we don't have a pseudo-program written to run child hooks and launch a process like on Linux, we're just launching the process directly).
Hum... on second though, there appears to be no equivalent for a `UNSET_CLOEXEC` ChildHook on Windows.
(This comment also applies to the following review: https://reviews.apache.org/r/65469/ )
We still want to minimize the time each FD spends in an inheritable state, in order to minimize leaks to other forks/processes in other threads. Ideally, any calls to this method should be made right before calls to `subprocess`. The Linux code does this implicitly, by __not__ cloexec-ing certain FDs in some methods.
Correct me if I'm wrong, but FDs do not need to be inheritable in order for `CreateProcessW` to use them as stdout/err FDs.
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65467/#review196932
-----------------------------------------------------------
On Feb. 2, 2018, 12:13 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65467/
> -----------------------------------------------------------
>
> (Updated Feb. 2, 2018, 12:13 p.m.)
>
>
> Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu.
>
>
> Bugs: MESOS-8512
> https://issues.apache.org/jira/browse/MESOS-8512
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This function is used on Windows to explicitly enable inheritance on a
> handle.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/internal/windows/inherit.hpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/65467/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 65467: Windows: Added
`internal::windows::enable_inherit(WindowsFD)`.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65467/#review196932
-----------------------------------------------------------
3rdparty/stout/include/stout/internal/windows/inherit.hpp
Lines 31-43 (patched)
<https://reviews.apache.org/r/65467/#comment276923>
This is basically what the ChildHook `UNSET_CLOEXEC` (libprocess/src/subprocess.cpp) should be doing.
- Joseph Wu
On Feb. 2, 2018, 12:13 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65467/
> -----------------------------------------------------------
>
> (Updated Feb. 2, 2018, 12:13 p.m.)
>
>
> Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu.
>
>
> Bugs: MESOS-8512
> https://issues.apache.org/jira/browse/MESOS-8512
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This function is used on Windows to explicitly enable inheritance on a
> handle.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/internal/windows/inherit.hpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/65467/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 65467: Windows: Added
`internal::windows::enable_inherit(WindowsFD)`.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65467/
-----------------------------------------------------------
(Updated Feb. 2, 2018, 12:13 p.m.)
Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu.
Bugs: MESOS-8512
https://issues.apache.org/jira/browse/MESOS-8512
Repository: mesos
Description
-------
This function is used on Windows to explicitly enable inheritance on a
handle.
Diffs
-----
3rdparty/stout/include/stout/internal/windows/inherit.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/65467/diff/1/
Testing
-------
Thanks,
Andrew Schwartzmeyer