You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Ben Mahler <be...@gmail.com> on 2013/06/28 05:33:39 UTC
Review Request 12146: Added os::sendfile that masks SIGPIPE.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12146/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman 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
Re: Review Request 12146: Added os::sendfile that masks SIGPIPE.
Posted by Ben Mahler <be...@gmail.com>.
> On July 2, 2013, 7:05 p.m., Vinod Kone wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp, line 52
> > <https://reviews.apache.org/r/12146/diff/1/?file=312848#file312848line52>
> >
> > No error checking?
See benh's comment below.
> On July 2, 2013, 7:05 p.m., Vinod Kone wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp, line 43
> > <https://reviews.apache.org/r/12146/diff/1/?file=312848#file312848line43>
> >
> > s/pending/_pending/ ? to avoid shadowing the member variable.
N/A now.
> On July 2, 2013, 7:05 p.m., Vinod Kone wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp, lines 54-55
> > <https://reviews.apache.org/r/12146/diff/1/?file=312848#file312848line54>
> >
> > I'm a bit confused. What is the difference between pending and blocked? Your comment above #39 seems to suggest pending => blocked? If yes, why do we need 'unblocked'?
This has been refactored now and this should read a lot easier with the helpers.
> On July 2, 2013, 7:05 p.m., Vinod Kone wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp, lines 58-59
> > <https://reviews.apache.org/r/12146/diff/1/?file=312848#file312848line58>
> >
> > Why do you have to do this? Probably needs some comments.
Removed the errno preservation here.
> On July 2, 2013, 7:05 p.m., Vinod Kone wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/os/sendfile_tests.cpp, line 58
> > <https://reviews.apache.org/r/12146/diff/1/?file=312849#file312849line58>
> >
> > Why on the heap?
LOREM_IPSUM is a std::string so the size must be determined at run-time, did you have another way to create the buffer on the stack?
> On July 2, 2013, 7:05 p.m., Vinod Kone wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/os/sendfile_tests.cpp, line 12
> > <https://reviews.apache.org/r/12146/diff/1/?file=312849#file312849line12>
> >
> > How about subclassing TemporaryDirectoryTest? You can get rid of a lot of boiler plate below.
That lives inside mesos, are you suggesting creating one for stout tests as well?
> On July 2, 2013, 7:05 p.m., Vinod Kone wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp, line 138
> > <https://reviews.apache.org/r/12146/diff/1/?file=312848#file312848line138>
> >
> > why the cast?
length is a size_t and we need an off_t for sendfile on OS X.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12146/#review22674
-----------------------------------------------------------
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
>
>
Re: Review Request 12146: Added os::sendfile that masks SIGPIPE.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12146/#review22674
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp
<https://reviews.apache.org/r/12146/#comment46356>
// __linux__ ?
3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp
<https://reviews.apache.org/r/12146/#comment46358>
s/pending/_pending/ ? to avoid shadowing the member variable.
3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp
<https://reviews.apache.org/r/12146/#comment46360>
No error checking?
3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp
<https://reviews.apache.org/r/12146/#comment46367>
I'm a bit confused. What is the difference between pending and blocked? Your comment above #39 seems to suggest pending => blocked? If yes, why do we need 'unblocked'?
3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp
<https://reviews.apache.org/r/12146/#comment46359>
Why do you have to do this? Probably needs some comments.
3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp
<https://reviews.apache.org/r/12146/#comment46363>
Did you mean to say..
"If the signal was generated before supression, we do nothing. Otherwise, if the signal was generated after supression, then we clear it."
3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp
<https://reviews.apache.org/r/12146/#comment46366>
Why do you need this? A comment perhaps?
3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp
<https://reviews.apache.org/r/12146/#comment46364>
const
3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp
<https://reviews.apache.org/r/12146/#comment46368>
why the cast?
3rdparty/libprocess/3rdparty/stout/tests/os/sendfile_tests.cpp
<https://reviews.apache.org/r/12146/#comment46370>
How about subclassing TemporaryDirectoryTest? You can get rid of a lot of boiler plate below.
3rdparty/libprocess/3rdparty/stout/tests/os/sendfile_tests.cpp
<https://reviews.apache.org/r/12146/#comment46372>
new line
3rdparty/libprocess/3rdparty/stout/tests/os/sendfile_tests.cpp
<https://reviews.apache.org/r/12146/#comment46375>
Why on the heap?
- Vinod Kone
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
>
>
Re: Review Request 12146: Added os::sendfile that masks SIGPIPE.
Posted by Ben Mahler <be...@gmail.com>.
> On July 9, 2013, 6:22 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp, line 76
> > <https://reviews.apache.org/r/12146/diff/1/?file=312848#file312848line76>
> >
> > How can a signal in this threads signal queue be delivered to another thread? Have you seen this in practice?
See 2.4.1 here: http://pubs.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_04.html
Notably:
"Signals generated for the process shall be delivered to exactly one of those threads within the process which is in a call to a sigwait() function selecting that signal or has not blocked delivery of the signal. If there are no threads in a call to a sigwait() function selecting that signal, and if all threads within the process block delivery of the signal, the signal shall remain pending on the process until a thread calls a sigwait() function selecting that signal, a thread unblocks delivery of the signal, or the action associated with the signal is set to ignore the signal."
Beefed up the comment to point to this page.
> On July 9, 2013, 6:22 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/sendfile.hpp, lines 127-128
> > <https://reviews.apache.org/r/12146/diff/1/?file=312848#file312848line127>
> >
> > 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?
Partially fixed this one, I removed the Try to force the caller to treat this with -1 error semantics.
However, I do like the idea of providing a sendfile wrapper (if you look at what we had in 3rdparty/libprocess/src/config.hpp it's made generic with a macro and is only usable by libprocess).
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12146/#review22845
-----------------------------------------------------------
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
>
>
Re: Review Request 12146: Added os::sendfile that masks SIGPIPE.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
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
>
>
Re: Review Request 12146: Added os::sendfile that masks SIGPIPE.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12146/#review23226
-----------------------------------------------------------
Dropping these ones in favor of new reviews now that our git repo has moved.
- Ben Mahler
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
>
>
Re: Review Request 12146: Added os::sendfile that masks SIGPIPE.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
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