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/04 01:33:54 UTC
[kudu-CR] Ignore SIGPIPE earlier in startup process
Hello David Ribeiro Alves,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/6262
to review the following change.
Change subject: Ignore SIGPIPE earlier in startup process
......................................................................
Ignore SIGPIPE earlier in startup process
This change resolves a race during startup where we are not protected
from SIGPIPE from the time we start the process until the time we start
the squeasel web server, which sets the disposition of SIGPIPE to
SIG_IGN.
This also factors some of the signal-handling helper functions into a
new set of util files, signal.{h,cc}.
Change-Id: I040bd38ff31451ed9e25e7cf2127c869cf08a628
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/logging.cc
M src/kudu/util/logging.h
A src/kudu/util/signal.cc
A src/kudu/util/signal.h
M src/kudu/util/subprocess.cc
6 files changed, 97 insertions(+), 26 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/6262/1
--
To view, visit http://gerrit.cloudera.org:8080/6262
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I040bd38ff31451ed9e25e7cf2127c869cf08a628
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
[kudu-CR] Ignore SIGPIPE earlier in startup process
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: Ignore SIGPIPE earlier in startup process
......................................................................
Patch Set 1:
(1 comment)
> What's the plan for the client?
I thought we could deal with the client in a separate patch. The idea I had there was to add an option to KuduClientBuilder called ignore_sigpipe(bool) that defaults to true. If clients want to manage their own signal handling themselves, at a process- or thread- level, then they can set it to false when constructing the client. If it's set to true, it calls something like EnsureSigPipeIgnored().
http://gerrit.cloudera.org:8080/#/c/6262/1/src/kudu/util/signal.cc
File src/kudu/util/signal.cc:
PS1, Line 38: static GoogleOnceType once = GOOGLE_ONCE_INIT;
: GoogleOnceInit(&once, &IgnoreSigPipe);
> I think you just moved this code, but actually curious why we bother using
Yeah I did just move this code... I assumed it's to avoid a syscall when we think we don't need it. I'm a little ambivalent on it and do think avoiding a static GoogleOnce is good for testability, so I'm not opposed to removing this.
Actually, I would like to remove the call to this from Subprocess::Start() but I'm a little worried about opening us up to additional test flakiness because of that. On the other hand, the call there could be masking other issues.
--
To view, visit http://gerrit.cloudera.org:8080/6262
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I040bd38ff31451ed9e25e7cf2127c869cf08a628
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Ignore SIGPIPE earlier in startup process
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has submitted this change and it was merged.
Change subject: Ignore SIGPIPE earlier in startup process
......................................................................
Ignore SIGPIPE earlier in startup process
This change resolves a race during startup where we are not protected
from SIGPIPE from the time we start the process until the time we start
the squeasel web server, which sets the disposition of SIGPIPE to
SIG_IGN.
This also factors some of the signal-handling helper functions into a
new set of util files, signal.{h,cc}.
Change-Id: I040bd38ff31451ed9e25e7cf2127c869cf08a628
Reviewed-on: http://gerrit.cloudera.org:8080/6262
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <to...@apache.org>
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/logging.cc
M src/kudu/util/logging.h
A src/kudu/util/signal.cc
A src/kudu/util/signal.h
M src/kudu/util/subprocess.cc
M src/kudu/util/test_main.cc
7 files changed, 110 insertions(+), 36 deletions(-)
Approvals:
Todd Lipcon: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/6262
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I040bd38ff31451ed9e25e7cf2127c869cf08a628
Gerrit-PatchSet: 4
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: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Ignore SIGPIPE earlier in startup process
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/6262
to look at the new patch set (#3).
Change subject: Ignore SIGPIPE earlier in startup process
......................................................................
Ignore SIGPIPE earlier in startup process
This change resolves a race during startup where we are not protected
from SIGPIPE from the time we start the process until the time we start
the squeasel web server, which sets the disposition of SIGPIPE to
SIG_IGN.
This also factors some of the signal-handling helper functions into a
new set of util files, signal.{h,cc}.
Change-Id: I040bd38ff31451ed9e25e7cf2127c869cf08a628
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/logging.cc
M src/kudu/util/logging.h
A src/kudu/util/signal.cc
A src/kudu/util/signal.h
M src/kudu/util/subprocess.cc
M src/kudu/util/test_main.cc
7 files changed, 110 insertions(+), 36 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/6262/3
--
To view, visit http://gerrit.cloudera.org:8080/6262
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I040bd38ff31451ed9e25e7cf2127c869cf08a628
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: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Ignore SIGPIPE earlier in startup process
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Ignore SIGPIPE earlier in startup process
......................................................................
Patch Set 1:
(1 comment)
What's the plan for the client?
http://gerrit.cloudera.org:8080/#/c/6262/1/src/kudu/util/signal.cc
File src/kudu/util/signal.cc:
PS1, Line 38: static GoogleOnceType once = GOOGLE_ONCE_INIT;
: GoogleOnceInit(&once, &IgnoreSigPipe);
I think you just moved this code, but actually curious why we bother using a ONCE here. it's not harmful to set twice
--
To view, visit http://gerrit.cloudera.org:8080/6262
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I040bd38ff31451ed9e25e7cf2127c869cf08a628
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Ignore SIGPIPE earlier in startup process
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Ignore SIGPIPE earlier in startup process
......................................................................
Patch Set 3: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6262/3/src/kudu/util/subprocess.cc
File src/kudu/util/subprocess.cc:
Line 22: #include <signal.h>
is this still necessary?
--
To view, visit http://gerrit.cloudera.org:8080/6262
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I040bd38ff31451ed9e25e7cf2127c869cf08a628
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: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Ignore SIGPIPE earlier in startup process
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/6262
to look at the new patch set (#2).
Change subject: Ignore SIGPIPE earlier in startup process
......................................................................
Ignore SIGPIPE earlier in startup process
This change resolves a race during startup where we are not protected
from SIGPIPE from the time we start the process until the time we start
the squeasel web server, which sets the disposition of SIGPIPE to
SIG_IGN.
This also factors some of the signal-handling helper functions into a
new set of util files, signal.{h,cc}.
Change-Id: I040bd38ff31451ed9e25e7cf2127c869cf08a628
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/logging.cc
M src/kudu/util/logging.h
A src/kudu/util/signal.cc
A src/kudu/util/signal.h
M src/kudu/util/subprocess.cc
M src/kudu/util/test_main.cc
7 files changed, 113 insertions(+), 36 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/6262/2
--
To view, visit http://gerrit.cloudera.org:8080/6262
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I040bd38ff31451ed9e25e7cf2127c869cf08a628
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: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Ignore SIGPIPE earlier in startup process
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Ignore SIGPIPE earlier in startup process
......................................................................
Patch Set 1:
(3 comments)
> > What's the plan for the client?
>
> I thought we could deal with the client in a separate patch. The
> idea I had there was to add an option to KuduClientBuilder called
> ignore_sigpipe(bool) that defaults to true. If clients want to
> manage their own signal handling themselves, at a process- or
> thread- level, then they can set it to false when constructing the
> client. If it's set to true, it calls something like
> EnsureSigPipeIgnored().
I don't think we even have to go that far; documenting that it's the application's responsibility to ignore SIGPIPE before setting up a TLS-enabled KuduClient should suffice. Unlike ignore_sigpipe(bool), this can be removed in the future once we do the BIO wrapper thing (mentioned in a different comment).
We should also find out whether Impala is vulnerable to this. Like us, squeasel's sq_start() will configure SIGPIPE to SIG_IGN, but also like us, it's possible that they may have called send() on TLS sockets by then.
http://gerrit.cloudera.org:8080/#/c/6262/1/src/kudu/util/logging.cc
File src/kudu/util/logging.cc:
Line 269: // Ignore SIGPIPE early in the startup process so that threads writing to TLS
Can you file a JIRA about this? Last night Todd mentioned that we can implement a BIO wrapper for openssl that'll let us explicitly set MSG_NOSIGNAL in send() calls. Once that happens, I'd argue we should remove this code.
http://gerrit.cloudera.org:8080/#/c/6262/1/src/kudu/util/logging.h
File src/kudu/util/logging.h:
PS1, Line 262: ignore
Nit: SIG_IGN, to be more precise.
http://gerrit.cloudera.org:8080/#/c/6262/1/src/kudu/util/subprocess.cc
File src/kudu/util/subprocess.cc:
Line 76: typedef sighandler_t SignalHandlerCallback;
Is this still needed?
--
To view, visit http://gerrit.cloudera.org:8080/6262
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I040bd38ff31451ed9e25e7cf2127c869cf08a628
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: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Ignore SIGPIPE earlier in startup process
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: Ignore SIGPIPE earlier in startup process
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/6262/1/src/kudu/util/logging.cc
File src/kudu/util/logging.cc:
Line 269: // Ignore SIGPIPE early in the startup process so that threads writing to TLS
> Can you file a JIRA about this? Last night Todd mentioned that we can imple
Filed KUDU-1910. However we use pipe() in subprocess.cc so there is an argument for keeping SIGPIPE ignored permanently.
http://gerrit.cloudera.org:8080/#/c/6262/1/src/kudu/util/logging.h
File src/kudu/util/logging.h:
PS1, Line 262: SIG_IG
> Nit: SIG_IGN, to be more precise.
Done
http://gerrit.cloudera.org:8080/#/c/6262/1/src/kudu/util/signal.cc
File src/kudu/util/signal.cc:
PS1, Line 38: }
:
> Yeah I did just move this code... I assumed it's to avoid a syscall when we
I think I have a solution involving blocking SIGPIPE from particular threads, but I'll leave that as a follow-up patch.
http://gerrit.cloudera.org:8080/#/c/6262/1/src/kudu/util/subprocess.cc
File src/kudu/util/subprocess.cc:
Line 76: #else
> Is this still needed?
Done
--
To view, visit http://gerrit.cloudera.org:8080/6262
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I040bd38ff31451ed9e25e7cf2127c869cf08a628
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: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Ignore SIGPIPE earlier in startup process
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: Ignore SIGPIPE earlier in startup process
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6262/3/src/kudu/util/subprocess.cc
File src/kudu/util/subprocess.cc:
Line 22: #include <signal.h>
> is this still necessary?
Turns out yes; we need it for kill()
--
To view, visit http://gerrit.cloudera.org:8080/6262
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I040bd38ff31451ed9e25e7cf2127c869cf08a628
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: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes