Re: Review Request 65467: Windows: Added `internal::windows::enable_inherit(WindowsFD)`.

[Andrew Schwartzmeyer]

> On Feb. 6, 2018, 1:55 p.m., Joseph Wu wrote:
> > 3rdparty/stout/include/stout/internal/windows/inherit.hpp
> > Lines 31-43 (patched)
> > 
> >
> > 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)`.

[Joseph Wu]

> On Feb. 6, 2018, 1:55 p.m., Joseph Wu wrote:
> > 3rdparty/stout/include/stout/internal/windows/inherit.hpp
> > Lines 31-43 (patched)
> > 
> >
> > 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)`.

[Andrew Schwartzmeyer]

> On Feb. 6, 2018, 1:55 p.m., Joseph Wu wrote:
> > 3rdparty/stout/include/stout/internal/windows/inherit.hpp
> > Lines 31-43 (patched)
> > 
> >
> > 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)`.

[Joseph Wu]

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


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)`.

[Andrew Schwartzmeyer]

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