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