----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/12146/#review22845 -----------------------------------------------------------
The suppressor abstraction is awesome! A few thoughts on helpers and os::sendfile below. 3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp <https://reviews.apache.org/r/12146/#comment46576> How about some helpers: namespace signals { bool pending(int signal) { sigset_t set; sigemptyset(&set); sigpending(&set); return sigismember(&set, signal); } // Returns true if the signal has been blocked or false if the signal was already blocked. bool block(int signal) { sigset_t set; sigaddset(&set, signal); sigset_t oldset; sigemptyset(&oldset); // Ignoring errors because ... pthread_sigmask(SIG_BLOCK, &set, &oldset); return !sigismember(&oldset, signal); } // Returns true if the signal has been unblocked or false if the signal was not previously blocked. bool unblock(int signal) { sigset_t set; sigaddset(&set, signal); sigset_t oldset; sigemptyset(&oldset); // Ignoring errors because ... pthread_sigmask(SIG_UNBLOCK, &set, &oldset); return sigismember(&oldset, signal); } } // namespace signals { Then in Suppressor(): -------- pending = signals::pending(signal); if (pending) { unblock = signals::block(signal); } -------- And in ~Suppressor(): -------- if (!pending && signals::pending(signal)) { ... clear signal ... } if (unblock) { signals::unblock(signal); } -------- Also, I'm happy to have you ignore the errors here because the only errors are if we pass a bad pointer, which should never happen unless the process or kernel is completely borked. That being said, we should add a comment that address this. 3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp <https://reviews.apache.org/r/12146/#comment46577> I see why you're saving errno (at least for ~Suppressor() and os::sendfile below), but I agree a comment here and in ~Supressor() would be great 3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp <https://reviews.apache.org/r/12146/#comment46578> How can a signal in this threads signal queue be delivered to another thread? Have you seen this in practice? 3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp <https://reviews.apache.org/r/12146/#comment46579> +1 for comment. 3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp <https://reviews.apache.org/r/12146/#comment46580> You can kill this with the helpers. 3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp <https://reviews.apache.org/r/12146/#comment46581> s/suppressed/suppress/ The former sounds like a "check", the latter sounds like an "action". I know that I decided to call it 'synchronized' instead of 'synchronize', but I was just following Java's lead. In this case, I think 'suppress' sounds better to me, thoughts? 3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp <https://reviews.apache.org/r/12146/#comment46582> The implication here is that callers will actually want to look at errno after calling os::sendfile. We have discussed these semantics in the past, but this would be (to the best of my knowledge) the first real function that returns a Try where we preserve the errno semantics and expect people to use them. So: why return a Try at all? Or why not just use 'suppress(SIGPIPE) {' directly where we currently call ::sendfile and already are properly handling all the errors? - Benjamin Hindman On June 28, 2013, 3:33 a.m., Ben Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/12146/ > ----------------------------------------------------------- > > (Updated June 28, 2013, 3:33 a.m.) > > > Review request for mesos, Benjamin Hindman, Ian Downes, and Vinod Kone. > > > Bugs: MESOS-508 > https://issues.apache.org/jira/browse/MESOS-508 > > > Repository: mesos > > > Description > ------- > > This is to resolve MESOS-508 by masking SIGPIPE per-thread through a > "Suppressor" abstraction. > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/Makefile.am > ade668846135bae9e977b65ce0ad0b72865cbf7a > 3rdparty/libprocess/3rdparty/stout/Makefile.am > 864b30ea5661cc7f613fdd422fe55f76fdafd1c1 > 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp > 429039644832470f7fd2eac19b213905cc81dcd3 > 3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp > PRE-CREATION > 3rdparty/libprocess/3rdparty/stout/tests/os/sendfile_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/12146/diff/ > > > Testing > ------- > > ./3rdparty/libprocess/3rdparty/stout-tests > --gtest_filter="OsSendfileTest.sendfile" --gtest_repeat=30000 > --gtest_break_on_failure > > > Thanks, > > Ben Mahler > >
