> On June 27, 2016, 4:06 p.m., Joris Van Remoortere wrote: > > 3rdparty/libprocess/src/libevent_ssl_socket.cpp, lines 808-810 > > <https://reviews.apache.org/r/49280/diff/1/?file=1431237#file1431237line808> > > > > I would have expected this note to appear above `evconnlistener_new`. > > > > Here I would expect a note explaining that a library implementation > > should not be opinionated about `cloexec`, and rather delegate that > > decision to the user. > > Take the linux socket as an example: > > http://man7.org/linux/man-pages/man2/socket.2.html > > > > We should either: > > 1) parameterize the socket interface to expose this option, and maybe > > provide the default that is most commonly used by libprocess. On the flip > > side you should argue that the default should be the one of least surprise, > > which would be *not* to provide `cloexec` in line with the linux API. > > 2) add a `TODO` here that explains this predicament, and point to the > > JIRA to take on this cleanup work. > > > > I propose #2 as we're trying to fix a bug before a release, and may not > > spend sufficient time to think through #1 correctly. > > This also seems like something that would be a valuable contribution by > > a community member. > > > > The same would apply for the `connect` code at line 489
For non-blocking, libprocess definitely assumes/relies-on FD's being non-blocking. I'll add the TODOs for optional `CLOEXEC` in a separate review, as there are a couple of files that would like that TODO. > On June 27, 2016, 4:06 p.m., Joris Van Remoortere wrote: > > 3rdparty/libprocess/src/libevent_ssl_socket.cpp, lines 813-814 > > <https://reviews.apache.org/r/49280/diff/1/?file=1431237#file1431237line813> > > > > Is this behavior consistent with the other implementations? > > If an error occurs, we don't propagate it to the `acceptor`? > > I think a comment is necessary here to explain what behavior we're > > accomplishing. Looks like the error should be translated into an failure on the listening socket's `accept_queue`. That will make the behavior (of returning a `Failure` on cloexec failure) consistent with the poll socket. > On June 27, 2016, 4:06 p.m., Joris Van Remoortere wrote: > > 3rdparty/libprocess/src/libevent_ssl_socket.cpp, line 837 > > <https://reviews.apache.org/r/49280/diff/1/?file=1431237#file1431237line837> > > > > Since we're trying to consistently use `cloexec` in this implementation > > in the interim, why do we not need to use `LEV_OPT_CLOSE_ON_EXEC` here? As BenM says below (same time :), the `LEV_OPT_CLOSE_ON_EXEC` flag is interpreted by libevent in 2.1.5-beta, whereas the recommended version of libevent is 2.0.22+. I'll add a note here instead. - Joseph ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49280/#review139683 ----------------------------------------------------------- On June 27, 2016, 6:34 p.m., Joseph Wu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49280/ > ----------------------------------------------------------- > > (Updated June 27, 2016, 6:34 p.m.) > > > Review request for mesos, Avinash sridharan, Benjamin Mahler, Artem > Harutyunyan, and Joris Van Remoortere. > > > Bugs: MESOS-5723 > https://issues.apache.org/jira/browse/MESOS-5723 > > > Repository: mesos > > > Description > ------- > > Incoming sockets are leaked when the agent forks because incoming > sockets are not modified with the CLOEXEC option. > > > Diffs > ----- > > 3rdparty/libprocess/src/libevent_ssl_socket.cpp > 3f80e863fdbb62d4cf7ce1b46c468a09f0694f56 > > Diff: https://reviews.apache.org/r/49280/diff/ > > > Testing > ------- > > make check (Centos7) > > > Thanks, > > Joseph Wu > >
