----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41632/#review121892 -----------------------------------------------------------
I've left a bunch of comments here, because I'd like us to nudge us towards learning from the style issues we've been having. :) But, I'd actually like to suggests a different approach. I agree with Joris that having a `signaledWrapper` inside the header is not ideal. As he said, in general, we should not keep global state in Stout headers, because Stout is an independent API. It is true that `signals.hpp` does call out to the kernel, which affects global state, but it does not itself _maintain_ that state in the header. So to me it would be appropriate to have functions that change the global state (as `signals.hpp` itself does), but not to _introduce_ global state here. So, I'd like to suggest a couple ways we might change this patch to accomplish these goals. My favorite option is 3. Before I mention them, I think it's worth considering the history of an API that's pretty successful, which has similar semantics: glog. glog exposes `google::InstallFailureSignalHandler`, which is forked between Windows and POSIX implementations, and which controls the semantics _of that signal handling for modules that are using glog_. I like this because the semantics are controlled by the application that is including glog -- an application can call this function and they will get specific behavior for glog. In our case, we 1. Try to remove the `signaledWrapper` from the `signalhandler.hpp` headers. I'm not sure how to do this, or if it's possible. 2. Perhaps consider making an `attachSignalHandler` API in libprocess. Although it is true that `signals.hpp` is in Stout, libprocess has much more precedent tracking state associated with a process. It could make sense that Stout continue to have `signals.hpp`, but have signal _attaching_ handled by libprocess. It's not entirely clear to me that this is the best place to track this state, but it is a plausible fit. 3. Perhaps put a function like `attachSlaveSigIntHandler` inside the slave. At the `HEAD` of the Windows integration branch it looks like we're using `signalConfigure` exactly once, in the slave. If this is a one-off thing (as it looks like it might be) then I think one option is to just move this directly to the slave package. If we don't need the customizability, then perhaps it's not worth the abstraction? What do you all think of this? Anything I'm missing? 3rdparty/libprocess/3rdparty/stout/include/Makefile.am (line 94) <https://reviews.apache.org/r/41632/#comment183741> Can we please make the `\ ` characters line up in RB (rather than in git). There are two other instances of this in this file. 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/signalhandler.hpp (line 24) <https://reviews.apache.org/r/41632/#comment183812> Is this used? 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/signalhandler.hpp (line 29) <https://reviews.apache.org/r/41632/#comment183742> Mesos project style mandates we do not indent inside a namespace. Can we please pull this back 2 spaces? 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/signalhandler.hpp (lines 31 - 33) <https://reviews.apache.org/r/41632/#comment183755> Also it seems like this should actually be in an internal namespace? Perhaps `os::internal`? 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/signalhandler.hpp (line 32) <https://reviews.apache.org/r/41632/#comment183751> Stout uses snake case for everything. In this file, the public symbols at the very least need to change: `signaledWrapper`, `signalHandler`, `configureSignal`, and in the Windows version `CtrlHandler`. But, please also change the local variables too. :) 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/signalhandler.hpp (line 22) <https://reviews.apache.org/r/41632/#comment183804> Is this header used? 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/signalhandler.hpp (line 23) <https://reviews.apache.org/r/41632/#comment183805> I think you need `#include <stout/windows.hpp>`, right? 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/signalhandler.hpp (line 25) <https://reviews.apache.org/r/41632/#comment183746> Nit: in Mesos we put a space between namespace and code. 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/signalhandler.hpp (line 26) <https://reviews.apache.org/r/41632/#comment183757> minor style thing: should be a space between `signaledWrapper` and `CtrlHandler`. 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/signalhandler.hpp (line 27) <https://reviews.apache.org/r/41632/#comment183747> Interesting, does this need to be here? Usually we put this kind of stuff in `windows.hpp`. 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/signalhandler.hpp (line 28) <https://reviews.apache.org/r/41632/#comment183749> Seems like we actually want this function to go in an internal namespace, like `os::internal`? What do you think? 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/signalhandler.hpp (line 44) <https://reviews.apache.org/r/41632/#comment183748> Can we get rid of the trailing whitespace here? - Alex Clemmer On March 3, 2016, 3 a.m., Daniel Pravat wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/41632/ > ----------------------------------------------------------- > > (Updated March 3, 2016, 3 a.m.) > > > Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, > Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun. > > > Bugs: MESOS-3644 > https://issues.apache.org/jira/browse/MESOS-3644 > > > Repository: mesos > > > Description > ------- > > Windows: Forked signal handling in `signalhandler.hpp`. > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/stout/include/Makefile.am > 03eff5a831283f6d298e9a1feecfdc7369cacfe7 > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/signalhandler.hpp > PRE-CREATION > 3rdparty/libprocess/3rdparty/stout/include/stout/os/signalhandler.hpp > PRE-CREATION > > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/signalhandler.hpp > PRE-CREATION > > Diff: https://reviews.apache.org/r/41632/diff/ > > > Testing > ------- > > OSX: make check > Windows: make > > > Thanks, > > Daniel Pravat > >
