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 65574: Windows: Fixed handle inheritance in `create_process` wrapper.
Date Wed, 21 Feb 2018 00:10:43 GMT


> On Feb. 20, 2018, 4:01 p.m., Akash Gupta wrote:
> > 3rdparty/stout/include/stout/os/windows/shell.hpp
> > Lines 275 (patched)
> > <https://reviews.apache.org/r/65574/diff/1/?file=1954645#file1954645line282>
> >
> >     It seems like you can have a race condition here if you send the same handles
to two different processes at the same time. If the second process finishes executing this
and then the first process exits and sets the handles to uninheritable, then you would set
uninheritable handles to the child process. If this function is supposed to have this behavior,
maybe add a comment explaining that this function mutates the handles, so the caller needs
to make sure to synchronize these calls.

> if you send the same handles to two different processes at the same time

This doesn't seem to be a valid scenario to me. When might we want to spawn more than one
process that are each supposed to inherit the same set of handles?

If we needed two processes to inherit the same handles, it would make sense to duplicate those
handles. But yeah we can comment it.


> On Feb. 20, 2018, 4:01 p.m., Akash Gupta wrote:
> > 3rdparty/stout/include/stout/os/windows/shell.hpp
> > Line 294 (original), 301 (patched)
> > <https://reviews.apache.org/r/65574/diff/1/?file=1954645#file1954645line309>
> >
> >     Hm, I remember we discussed something like `CreateProcessW` does process initialization
async. Do we need to wait for the process to be initialized?

I don't believe we need to wait, as inheriting the handles does not seem to be part of initialization.
It's hard to be sure, as the docs don't state much about it, but what they do say is this:

> Note that the function returns before the process has finished initialization. If a required
DLL cannot be located or fails to initialize, the process is terminated.

I think the docs are trying to say that `CreateProcess` returns before the other process has
"finished" starting. But I don't think this would have any bearing on handles. It would seem
to me that the system call to `CreateProcess` processes the handle inheritance, making any
handle that was inheritable at the point the call was made, inherited in the child process.
I do not think the child process has to "initialize" its inheritance of those handles.

In any case, it's not particularly clear. I'd say we should make sure to add a comment that
there may be an issue here, and then come back to it if we it actually manifests.


- Andrew


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65574/#review197823
-----------------------------------------------------------


On Feb. 20, 2018, 11:39 a.m., Andrew Schwartzmeyer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65574/
> -----------------------------------------------------------
> 
> (Updated Feb. 20, 2018, 11:39 a.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jeff Coffler, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-6838 and MESOS-8512
>     https://issues.apache.org/jira/browse/MESOS-6838
>     https://issues.apache.org/jira/browse/MESOS-8512
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The primary change in this patch is that `create_process` now enables
> inheritance for the `pipe` handles passed before starting the child
> process. This is required, otherwise the child process will behave
> incorrectly (for example, it will write to `stdout` but that will go
> nowhere, as the redirection silently failed). After the process is
> created, inheritance is disabled to prevent further calls to
> `create_process` from inheriting the wrong handles.
> 
> The `std::tuple<os::WindowsFD, ...>` type was changed to a
> `std::array<os::WindowsFD, 3>` as it is significantly easier to work
> with (it supports iteration).
> 
> 
> Diffs
> -----
> 
>   3rdparty/stout/include/stout/os/windows/shell.hpp 542039c31f94eda1af121335b12edf9b83725ae5

> 
> 
> Diff: https://reviews.apache.org/r/65574/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>


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