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 65467: Windows: Added `internal::windows::enable_inherit(WindowsFD)`.
Date Wed, 07 Feb 2018 19:02:43 GMT


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


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