You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Mike Percy (Code Review)" <ge...@cloudera.org> on 2017/03/07 03:15:46 UTC
[kudu-CR] subprocess: Don't require process-wide SIG IGN for SIGPIPE
Hello David Ribeiro Alves, Adar Dembo, Todd Lipcon,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/6281
to review the following change.
Change subject: subprocess: Don't require process-wide SIG_IGN for SIGPIPE
......................................................................
subprocess: Don't require process-wide SIG_IGN for SIGPIPE
This patch implements an alternative approach to handling SIGPIPE while
forking a subprocess that doesn't require a process-wide signal
disposition of SIG_IGN for SIGPIPE.
The way this patch works is that the thread that may generate a SIGPIPE
blocks the PIPE signal in its per-thread signal mask, performs the
potentially signal-inducing write() call on a pipe, consumes a
potentially pending SIGPIPE notification, and unblocks the delivery of
SIGPIPE.
This per-thread behavior is possible because SIGPIPE is delivered to the
thread that generated it. From the POSIX.1 spec section 2.10.14 Signals:
The SIGPIPE signal shall be sent to a thread that attempts to send
data on a socket that is no longer able to send (one that is no longer
connected), except that the signal is suppressed if the MSG_NOSIGNAL
flag is used in calls to send(), sendto(), and sendmsg(). Regardless
of whether the generation of the signal is suppressed, the send
operation shall fail with the [EPIPE] error.
While it is documented as such for sockets, it appears to work the same
way for pipes.
Using this approach in general has the benefit that clients do not have
to set the disposition of SIGPIPE to SIG_IGN process-wide in order to
block signals we are only concerned about for specific threads.
Change-Id: I4d10207e1bc09493bbf9131d4e622ace895d2186
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/logging.cc
A src/kudu/util/signal-test.cc
M src/kudu/util/signal.cc
M src/kudu/util/signal.h
M src/kudu/util/subprocess.cc
M src/kudu/util/subprocess.h
M src/kudu/util/test_main.cc
8 files changed, 269 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/6281/1
--
To view, visit http://gerrit.cloudera.org:8080/6281
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I4d10207e1bc09493bbf9131d4e622ace895d2186
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] subprocess: Don't require process-wide SIG IGN for SIGPIPE
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6281
to look at the new patch set (#3).
Change subject: subprocess: Don't require process-wide SIG_IGN for SIGPIPE
......................................................................
subprocess: Don't require process-wide SIG_IGN for SIGPIPE
This patch implements an alternative approach to handling SIGPIPE while
forking a subprocess that doesn't require a process-wide signal
disposition of SIG_IGN for SIGPIPE.
This patch also fixes a bug in Subprocess::Call() where we were not
returning an IOError when we could not write our full stdin, due to
coercion of ssize_t bytes_written (negative value) to size_t in a
comparison.
The way this patch works is that the thread that may generate a SIGPIPE
blocks the PIPE signal in its per-thread signal mask, performs the
potentially signal-inducing write() call on a pipe, consumes a
potentially pending SIGPIPE notification, and unblocks the delivery of
SIGPIPE.
This per-thread behavior is possible because SIGPIPE is delivered to the
thread that generated it. From the POSIX.1 spec section 2.10.14 Signals:
The SIGPIPE signal shall be sent to a thread that attempts to send
data on a socket that is no longer able to send (one that is no longer
connected), except that the signal is suppressed if the MSG_NOSIGNAL
flag is used in calls to send(), sendto(), and sendmsg(). Regardless
of whether the generation of the signal is suppressed, the send
operation shall fail with the [EPIPE] error.
While it is documented as such for sockets, it appears to work the same
way for pipes.
Using this approach in general has the benefit that clients do not have
to set the disposition of SIGPIPE to SIG_IGN process-wide in order to
block signals we are only concerned about for specific threads.
Change-Id: I4d10207e1bc09493bbf9131d4e622ace895d2186
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/logging.cc
A src/kudu/util/signal-test.cc
M src/kudu/util/signal.cc
M src/kudu/util/signal.h
M src/kudu/util/subprocess-test.cc
M src/kudu/util/subprocess.cc
M src/kudu/util/subprocess.h
M src/kudu/util/test_main.cc
9 files changed, 288 insertions(+), 19 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/6281/3
--
To view, visit http://gerrit.cloudera.org:8080/6281
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4d10207e1bc09493bbf9131d4e622ace895d2186
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] subprocess: Don't require process-wide SIG IGN for SIGPIPE
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has abandoned this change.
Change subject: subprocess: Don't require process-wide SIG_IGN for SIGPIPE
......................................................................
Abandoned
I think the plan is to go another direction for avoiding SIGPIPE due to the TLS lib.
--
To view, visit http://gerrit.cloudera.org:8080/6281
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: I4d10207e1bc09493bbf9131d4e622ace895d2186
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] subprocess: Don't require process-wide SIG IGN for SIGPIPE
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6281
to look at the new patch set (#2).
Change subject: subprocess: Don't require process-wide SIG_IGN for SIGPIPE
......................................................................
subprocess: Don't require process-wide SIG_IGN for SIGPIPE
This patch implements an alternative approach to handling SIGPIPE while
forking a subprocess that doesn't require a process-wide signal
disposition of SIG_IGN for SIGPIPE.
This patch also fixes a bug in Subprocess::Call() where we were not
returning an IOError when we could not write our full stdin, due to
coercion of ssize_t bytes_written (negative value) to size_t in a
comparison.
The way this patch works is that the thread that may generate a SIGPIPE
blocks the PIPE signal in its per-thread signal mask, performs the
potentially signal-inducing write() call on a pipe, consumes a
potentially pending SIGPIPE notification, and unblocks the delivery of
SIGPIPE.
This per-thread behavior is possible because SIGPIPE is delivered to the
thread that generated it. From the POSIX.1 spec section 2.10.14 Signals:
The SIGPIPE signal shall be sent to a thread that attempts to send
data on a socket that is no longer able to send (one that is no longer
connected), except that the signal is suppressed if the MSG_NOSIGNAL
flag is used in calls to send(), sendto(), and sendmsg(). Regardless
of whether the generation of the signal is suppressed, the send
operation shall fail with the [EPIPE] error.
While it is documented as such for sockets, it appears to work the same
way for pipes.
Using this approach in general has the benefit that clients do not have
to set the disposition of SIGPIPE to SIG_IGN process-wide in order to
block signals we are only concerned about for specific threads.
Change-Id: I4d10207e1bc09493bbf9131d4e622ace895d2186
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/logging.cc
A src/kudu/util/signal-test.cc
M src/kudu/util/signal.cc
M src/kudu/util/signal.h
M src/kudu/util/subprocess-test.cc
M src/kudu/util/subprocess.cc
M src/kudu/util/subprocess.h
M src/kudu/util/test_main.cc
9 files changed, 310 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/6281/2
--
To view, visit http://gerrit.cloudera.org:8080/6281
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4d10207e1bc09493bbf9131d4e622ace895d2186
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>