You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by James Peach <jp...@apache.org> on 2018/09/06 19:19:00 UTC
Review Request 68660: Disabled SIGCHLD handling in the libev event
loop.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68660/
-----------------------------------------------------------
Review request for mesos, Benjamin Bannier, Till Toenshoff, and Vinod Kone.
Bugs: MESOS-9212
https://issues.apache.org/jira/browse/MESOS-9212
Repository: mesos
Description
-------
libev will consume SIGCHLD signals by default, which interferes
with a number of libprocess assumptions around dealing with child
processes. The recommended way to disable this behavior is to reset
the SIGCHLD signal handler after initializing the libeve default
event loop.
Diffs
-----
3rdparty/libprocess/src/posix/libev/libev.cpp 173ee466e61d3a754202213b94645e125ce0421a
Diff: https://reviews.apache.org/r/68660/diff/1/
Testing
-------
configure with --with-libev=/usr, make check (Fedora 28)
Thanks,
James Peach
Re: Review Request 68660: Disabled SIGCHLD handling in the libev
event loop.
Posted by James Peach <jp...@apache.org>.
> On Sept. 6, 2018, 10:47 p.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/src/posix/libev/libev.cpp
> > Lines 84-89 (patched)
> > <https://reviews.apache.org/r/68660/diff/1/?file=2086293#file2086293line84>
> >
> > * Let's use sigaction here rather than ::signal? See the portability notes here: http://man7.org/linux/man-pages/man2/signal.2.html
> >
> > * Why do you need to set SIG_DFL? Can we just retrieve the old value and restore it later (note that sigaction makes this possible).
> >
> > * Where's the race here? Can you clarify?
> >
> > * Let's also remove the libev patch file if we have this proper approach to disabling it?
> >
> > * Note that we have some stuff in os/signals.hpp, I don't know if you want to extend that stuff or not, either way seems fine to me.
> Let's use sigaction here rather than ::signal? See the portability notes here: http://man7.org/linux/man-pages/man2/signal.2.html
> Why do you need to set SIG_DFL? Can we just retrieve the old value and restore it later (note that sigaction makes this possible).
Setting `SIG_DFL` is not important, I'm just trying to sample the old handler value. You are right about using `sigaction` though, that's a better approach.
> Where's the race here? Can you clarify?
Just generically mentioning that manipulating the signal handling is racey.
> Let's also remove the libev patch file if we have this proper approach to disabling it?
The patch causes the whole feature to be compiled out of libev, so it still results in a benefit (smaller code).
> Note that we have some stuff in os/signals.hpp, I don't know if you want to extend that stuff or not, either way seems fine to me.
I'll probably use `sigaction` directly.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68660/#review208427
-----------------------------------------------------------
On Sept. 6, 2018, 7:19 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68660/
> -----------------------------------------------------------
>
> (Updated Sept. 6, 2018, 7:19 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Till Toenshoff, and Vinod Kone.
>
>
> Bugs: MESOS-9212
> https://issues.apache.org/jira/browse/MESOS-9212
>
>
> Repository: mesos
>
>
> Description
> -------
>
> libev will consume SIGCHLD signals by default, which interferes
> with a number of libprocess assumptions around dealing with child
> processes. The recommended way to disable this behavior is to reset
> the SIGCHLD signal handler after initializing the libeve default
> event loop.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/posix/libev/libev.cpp 173ee466e61d3a754202213b94645e125ce0421a
>
>
> Diff: https://reviews.apache.org/r/68660/diff/1/
>
>
> Testing
> -------
>
> configure with --with-libev=/usr, make check (Fedora 28)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 68660: Disabled SIGCHLD handling in the libev
event loop.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68660/#review208427
-----------------------------------------------------------
3rdparty/libprocess/src/posix/libev/libev.cpp
Lines 84-89 (patched)
<https://reviews.apache.org/r/68660/#comment292338>
* Let's use sigaction here rather than ::signal? See the portability notes here: http://man7.org/linux/man-pages/man2/signal.2.html
* Why do you need to set SIG_DFL? Can we just retrieve the old value and restore it later (note that sigaction makes this possible).
* Where's the race here? Can you clarify?
* Let's also remove the libev patch file if we have this proper approach to disabling it?
* Note that we have some stuff in os/signals.hpp, I don't know if you want to extend that stuff or not, either way seems fine to me.
- Benjamin Mahler
On Sept. 6, 2018, 7:19 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68660/
> -----------------------------------------------------------
>
> (Updated Sept. 6, 2018, 7:19 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Till Toenshoff, and Vinod Kone.
>
>
> Bugs: MESOS-9212
> https://issues.apache.org/jira/browse/MESOS-9212
>
>
> Repository: mesos
>
>
> Description
> -------
>
> libev will consume SIGCHLD signals by default, which interferes
> with a number of libprocess assumptions around dealing with child
> processes. The recommended way to disable this behavior is to reset
> the SIGCHLD signal handler after initializing the libeve default
> event loop.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/posix/libev/libev.cpp 173ee466e61d3a754202213b94645e125ce0421a
>
>
> Diff: https://reviews.apache.org/r/68660/diff/1/
>
>
> Testing
> -------
>
> configure with --with-libev=/usr, make check (Fedora 28)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 68660: Disabled SIGCHLD handling in the libev
event loop.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68660/#review208455
-----------------------------------------------------------
PASS: Mesos patch 68660 was successfully built and tested.
Reviews applied: `['68660']`
All the build artifacts available at: http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2309/mesos-review-68660
- Mesos Reviewbot Windows
On Sept. 7, 2018, 4:46 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68660/
> -----------------------------------------------------------
>
> (Updated Sept. 7, 2018, 4:46 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Till Toenshoff, and Vinod Kone.
>
>
> Bugs: MESOS-9212
> https://issues.apache.org/jira/browse/MESOS-9212
>
>
> Repository: mesos
>
>
> Description
> -------
>
> libev will consume SIGCHLD signals by default, which interferes
> with a number of libprocess assumptions around dealing with child
> processes. The recommended way to disable this behavior is to reset
> the SIGCHLD signal handler after initializing the libeve default
> event loop.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/posix/libev/libev.cpp 173ee466e61d3a754202213b94645e125ce0421a
>
>
> Diff: https://reviews.apache.org/r/68660/diff/2/
>
>
> Testing
> -------
>
> configure with --with-libev=/usr, make check (Fedora 28)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 68660: Disabled SIGCHLD handling in the libev
event loop.
Posted by James Peach <jp...@apache.org>.
> On Sept. 11, 2018, 11:14 p.m., Benjamin Mahler wrote:
> > Looks good! We should remove our libev patch file now, since (1) it's confusing (there's no explanation of why we have it; reading it seems to suggest it's needed for correctness, but it's not) and (2) makes upgrading libev more challenging (need to potentially adjust the patch file). Can you take care of that?
I'll try ti unbundle libev altogether in [MESOS-895](https://issues.apache.org/jira/browse/MESOS-895)
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68660/#review208534
-----------------------------------------------------------
On Sept. 7, 2018, 4:46 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68660/
> -----------------------------------------------------------
>
> (Updated Sept. 7, 2018, 4:46 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Till Toenshoff, and Vinod Kone.
>
>
> Bugs: MESOS-9212
> https://issues.apache.org/jira/browse/MESOS-9212
>
>
> Repository: mesos
>
>
> Description
> -------
>
> libev will consume SIGCHLD signals by default, which interferes
> with a number of libprocess assumptions around dealing with child
> processes. The recommended way to disable this behavior is to reset
> the SIGCHLD signal handler after initializing the libeve default
> event loop.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/posix/libev/libev.cpp 173ee466e61d3a754202213b94645e125ce0421a
>
>
> Diff: https://reviews.apache.org/r/68660/diff/2/
>
>
> Testing
> -------
>
> configure with --with-libev=/usr, make check (Fedora 28)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 68660: Disabled SIGCHLD handling in the libev
event loop.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68660/#review208534
-----------------------------------------------------------
Ship it!
Looks good! We should remove our libev patch file now, since (1) it's confusing (there's no explanation of why we have it; reading it seems to suggest it's needed for correctness, but it's not) and (2) makes upgrading libev more challenging (need to potentially adjust the patch file). Can you take care of that?
- Benjamin Mahler
On Sept. 7, 2018, 4:46 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68660/
> -----------------------------------------------------------
>
> (Updated Sept. 7, 2018, 4:46 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Till Toenshoff, and Vinod Kone.
>
>
> Bugs: MESOS-9212
> https://issues.apache.org/jira/browse/MESOS-9212
>
>
> Repository: mesos
>
>
> Description
> -------
>
> libev will consume SIGCHLD signals by default, which interferes
> with a number of libprocess assumptions around dealing with child
> processes. The recommended way to disable this behavior is to reset
> the SIGCHLD signal handler after initializing the libeve default
> event loop.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/posix/libev/libev.cpp 173ee466e61d3a754202213b94645e125ce0421a
>
>
> Diff: https://reviews.apache.org/r/68660/diff/2/
>
>
> Testing
> -------
>
> configure with --with-libev=/usr, make check (Fedora 28)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 68660: Disabled SIGCHLD handling in the libev
event loop.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68660/
-----------------------------------------------------------
(Updated Sept. 7, 2018, 4:46 p.m.)
Review request for mesos, Benjamin Bannier, Till Toenshoff, and Vinod Kone.
Bugs: MESOS-9212
https://issues.apache.org/jira/browse/MESOS-9212
Repository: mesos
Description
-------
libev will consume SIGCHLD signals by default, which interferes
with a number of libprocess assumptions around dealing with child
processes. The recommended way to disable this behavior is to reset
the SIGCHLD signal handler after initializing the libeve default
event loop.
Diffs (updated)
-----
3rdparty/libprocess/src/posix/libev/libev.cpp 173ee466e61d3a754202213b94645e125ce0421a
Diff: https://reviews.apache.org/r/68660/diff/2/
Changes: https://reviews.apache.org/r/68660/diff/1-2/
Testing
-------
configure with --with-libev=/usr, make check (Fedora 28)
Thanks,
James Peach
Re: Review Request 68660: Disabled SIGCHLD handling in the libev
event loop.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68660/#review208423
-----------------------------------------------------------
PASS: Mesos patch 68660 was successfully built and tested.
Reviews applied: `['68660']`
All the build artifacts available at: http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2304/mesos-review-68660
- Mesos Reviewbot Windows
On Sept. 6, 2018, 7:19 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68660/
> -----------------------------------------------------------
>
> (Updated Sept. 6, 2018, 7:19 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Till Toenshoff, and Vinod Kone.
>
>
> Bugs: MESOS-9212
> https://issues.apache.org/jira/browse/MESOS-9212
>
>
> Repository: mesos
>
>
> Description
> -------
>
> libev will consume SIGCHLD signals by default, which interferes
> with a number of libprocess assumptions around dealing with child
> processes. The recommended way to disable this behavior is to reset
> the SIGCHLD signal handler after initializing the libeve default
> event loop.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/posix/libev/libev.cpp 173ee466e61d3a754202213b94645e125ce0421a
>
>
> Diff: https://reviews.apache.org/r/68660/diff/1/
>
>
> Testing
> -------
>
> configure with --with-libev=/usr, make check (Fedora 28)
>
>
> Thanks,
>
> James Peach
>
>