----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67137/#review203426 -----------------------------------------------------------
src/slave/containerizer/mesos/launch.cpp Lines 484-485 (patched) <https://reviews.apache.org/r/67137/#comment285636> For Windows _specifically_ this change is superfluous. All new processes created by Mesos go through our `create_process` wrapper, which with the completion of [MESOS-8926](https://issues.apache.org/jira/browse/MESOS-8926) will whitelist the three handles to be inherited, preventing any leaks. src/slave/containerizer/mesos/launch.cpp Lines 494 (patched) <https://reviews.apache.org/r/67137/#comment285637> Ah, in [#66834](https://reviews.apache.org/r/66834) I opted to specialize `flags` over implementating `operator<<` for Windows' `int_fd` abstraction (because the representation is chosen for Mesos, it is not standard). I can't think of way to safely implement the logic you're looking for here on Windows; it is impossible to tell from an arbitrary hexadecimal address whether or not that address is a `HANDLE` or a `SOCKET`. So anyway, suffice it to say I think the entire `getOpenFileDescriptors` logic should be under `#ifndef __WINDOWS__`, with a note that it's unnecessary due to whitelisting of inherited handles. src/slave/containerizer/mesos/launch.cpp Lines 1037 (patched) <https://reviews.apache.org/r/67137/#comment285638> Below we check that `containerStatusFd.isSome()` (and later close it if it was), so I don't think it's safe to `get()` here. - Andrew Schwartzmeyer On May 18, 2018, 5:36 a.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67137/ > ----------------------------------------------------------- > > (Updated May 18, 2018, 5:36 a.m.) > > > Review request for mesos, Gilbert Song, Jie Yu, and James Peach. > > > Bugs: MESOS-8917 > https://issues.apache.org/jira/browse/MESOS-8917 > > > Repository: mesos > > > Description > ------- > > Avoided leaking file descriptors in Mesos containerizer. > > > Diffs > ----- > > src/slave/containerizer/mesos/launch.cpp > f25d90651ef32495c9161c3eaed8a327d1b2b926 > > > Diff: https://reviews.apache.org/r/67137/diff/5/ > > > Testing > ------- > > `make check` > > I still need to confirm this patch in CI on a wider range of scenarios. > > > Thanks, > > Benjamin Bannier > >
