Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
460094:
Make POSIX SIGTERM/SIGINT/SIGHUP handler async signal safe. (Closed)
|
Created:
11 years ago by willchan no longer on Chromium Modified:
9 years, 7 months ago CC:
chromium-reviews_googlegroups.com Visibility:
Public. |
DescriptionMake POSIX SIGTERM/SIGINT/SIGHUP handler async signal safe.
* Don't use LOG/CHECK. Replace with RAW_LOG/DCHECK (newly added to logging.h)
* Don't directly post a task to the UI loop. Write to a magic pipe. Read this from a separate thread which will post to a task to the UI loop.
BUG=http://crbug.com/29240
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=34036
Patch Set 1 #
Total comments: 11
Patch Set 2 : Address comments. #
Total comments: 21
Patch Set 3 : Delete unused code. #Patch Set 4 : Address mark's comments. #
Total comments: 17
Patch Set 5 : Address more of mark's comments. #
Total comments: 1
Patch Set 6 : Remove unneeded semicolon. #
Total comments: 3
Patch Set 7 : move shutdown thread creation after UI thread is registered. #
Total comments: 1
Messages
Total messages: 20 (0 generated)
I was going to use a recurring select() task on the UI thread, but jar pointed out to me that this prevents the cpu from going idle, so I used a worker thread with a blocking read().
I have a couple of high-level comments in advance of doing a proper review. 1. I don't think it's appropriate to post a task that will run indefinitely to the worker pool. Since this will be a thread that sits on a blocking read() for effectively its entire life, we should unconditionally put it on its own thread managed outside of the worker pool system. We shouldn't be cluttering the worker pool with things that don't terminate. 2. You're still using snprintf in your "raw" logger, but snprintf isn't necessarily safe in an asynchronous signal handler either. Maybe it is on one platform, but is it on all of the platforms that this needs to run on? All of the platforms this will ultimately run on? I think you should just avoid any logging from the signal handler.
On 2009/12/06 16:58:12, Mark Mentovai wrote: > I have a couple of high-level comments in advance of doing a proper review. > > 1. I don't think it's appropriate to post a task that will run indefinitely to > the worker pool. Since this will be a thread that sits on a blocking read() for > effectively its entire life, we should unconditionally put it on its own thread > managed outside of the worker pool system. We shouldn't be cluttering the > worker pool with things that don't terminate. Fair enough. For some reason, I thought that WorkerPool and base::Thread were the only options here, but I forgot about PlatformThread. Is it fine with you if I use that? > > 2. You're still using snprintf in your "raw" logger, but snprintf isn't > necessarily safe in an asynchronous signal handler either. Maybe it is on one > platform, but is it on all of the platforms that this needs to run on? All of > the platforms this will ultimately run on? I think you should just avoid any > logging from the signal handler. That's a fair comment. How about I just disallow format specification then?
willchan@chromium.org wrote: > Fair enough. For some reason, I thought that WorkerPool and base::Thread were > the only options here, but I forgot about PlatformThread. Is it fine with you > if I use that? Yup. > That's a fair comment. How about I just disallow format specification then? You could do that, but I'm not convinced of the utility of the LOG statements inside the signal handler, which is why I'd also be OK with you just removing them. Mark
http://codereview.chromium.org/460094/diff/1/3 File base/logging.h (right): http://codereview.chromium.org/460094/diff/1/3#newcode784 base/logging.h:784: logging::RawLog(logging::LOG_FATAL, "Check failed: " #condition "\n", \ If RawLog takes a pattern and variable length arguments, shouldn't the arguments here include "%s" in the pattern (and not pass in the length?). (Note that #condition could contain % sequences.) http://codereview.chromium.org/460094/diff/1/4 File chrome/browser/browser_main.cc (right): http://codereview.chromium.org/460094/diff/1/4#newcode173 chrome/browser/browser_main.cc:173: int g_shutdown_pipe_write_fd = -1; static http://codereview.chromium.org/460094/diff/1/4#newcode174 chrome/browser/browser_main.cc:174: int g_shutdown_pipe_read_fd = -1; static http://codereview.chromium.org/460094/diff/1/4#newcode191 chrome/browser/browser_main.cc:191: RAW_CHECK(write(g_shutdown_pipe_write_fd, "\0", 1) == 1); HANDLE_EINTR?
Mark, I still found some of the logging messages useful, so I kept the RAW_LOG. If you feel strongly here, let me know. http://codereview.chromium.org/460094/diff/1/3 File base/logging.h (right): http://codereview.chromium.org/460094/diff/1/3#newcode784 base/logging.h:784: logging::RawLog(logging::LOG_FATAL, "Check failed: " #condition "\n", \ On 2009/12/07 18:56:09, agl wrote: > If RawLog takes a pattern and variable length arguments, shouldn't the arguments > here include "%s" in the pattern (and not pass in the length?). (Note that > #condition could contain % sequences.) Yes, that was wrong. It's irrelevant now since I'm getting rid of the pattern. http://codereview.chromium.org/460094/diff/1/4 File chrome/browser/browser_main.cc (right): http://codereview.chromium.org/460094/diff/1/4#newcode173 chrome/browser/browser_main.cc:173: int g_shutdown_pipe_write_fd = -1; On 2009/12/07 18:56:09, agl wrote: > static This is all within an anonymous namespace. Do you still want it to be static? http://codereview.chromium.org/460094/diff/1/4#newcode174 chrome/browser/browser_main.cc:174: int g_shutdown_pipe_read_fd = -1; On 2009/12/07 18:56:09, agl wrote: > static See previous comment. http://codereview.chromium.org/460094/diff/1/4#newcode191 chrome/browser/browser_main.cc:191: RAW_CHECK(write(g_shutdown_pipe_write_fd, "\0", 1) == 1); On 2009/12/07 18:56:09, agl wrote: > HANDLE_EINTR? Done.
http://codereview.chromium.org/460094/diff/1/4 File chrome/browser/browser_main.cc (right): http://codereview.chromium.org/460094/diff/1/4#newcode173 chrome/browser/browser_main.cc:173: int g_shutdown_pipe_write_fd = -1; On 2009/12/07 19:56:14, willchan wrote: > This is all within an anonymous namespace. No need in this case. (This is why I dislike anonymous namespaces.) http://codereview.chromium.org/460094/diff/1/4#newcode251 chrome/browser/browser_main.cc:251: fd_set read_fds; This appears to be unused?
http://codereview.chromium.org/460094/diff/1/4 File chrome/browser/browser_main.cc (right): http://codereview.chromium.org/460094/diff/1/4#newcode251 chrome/browser/browser_main.cc:251: fd_set read_fds; On 2009/12/07 20:01:05, agl wrote: > This appears to be unused? Deleted, thanks.
http://codereview.chromium.org/460094/diff/3001/3002 File base/logging.cc (right): http://codereview.chromium.org/460094/diff/3001/3002#newcode693 base/logging.cc:693: void RawLog(int lvl, const char* message) { Again with lvl. http://codereview.chromium.org/460094/diff/3001/3002#newcode696 base/logging.cc:696: const size_t buflen = strlen(message); buflen sounds like something that it's not, I'd call this message_len. http://codereview.chromium.org/460094/diff/3001/3002#newcode709 base/logging.cc:709: if (buflen > 0 && message[buflen-1] != '\n') { Spaces around the - also. http://codereview.chromium.org/460094/diff/3001/3003 File base/logging.h (right): http://codereview.chromium.org/460094/diff/3001/3003#newcode776 base/logging.h:776: void RawLog(int lvl, const char* message); Prefer real words like "level" to made-up ones like "lvl". http://codereview.chromium.org/460094/diff/3001/3004 File chrome/browser/browser_main.cc (right): http://codereview.chromium.org/460094/diff/3001/3004#newcode178 chrome/browser/browser_main.cc:178: RAW_CHECK(signal == expected_signal); I don't know if having expected_signal as an argument here makes much sense, since it's only used for the CHECK. It seems about as useful to just move these RAW_CHECKs to the SIG*Handlers below. http://codereview.chromium.org/460094/diff/3001/3004#newcode189 chrome/browser/browser_main.cc:189: RAW_CHECK(HANDLE_EINTR(write(g_shutdown_pipe_write_fd, "\0", 1) == 1)); Maybe we want to write the signal number to the pipe so it's available on the other end? http://codereview.chromium.org/460094/diff/3001/3004#newcode246 chrome/browser/browser_main.cc:246: : shutdown_fd_(shutdown_fd) { 4-space indent on this line, not 2. http://codereview.chromium.org/460094/diff/3001/3004#newcode259 chrome/browser/browser_main.cc:259: NOTREACHED() << "Unexpected error: " << strerror(errno); I guess there's no PNOTREACHED, huh? http://codereview.chromium.org/460094/diff/3001/3004#newcode402 chrome/browser/browser_main.cc:402: LOG(DFATAL) << "Failed to create pipe: " << strerror(errno); Use PLOG here, not LOG. PLOG does the strerror thing. http://codereview.chromium.org/460094/diff/3001/3004#newcode410 chrome/browser/browser_main.cc:410: LOG(DFATAL) << "Failed to create shutdown detector task."; Wrap this in {}s, because you've got a multi-line conditional and it'll be easier to read. The conditional is already kind of hard to read... http://codereview.chromium.org/460094/diff/3001/3004#newcode412 chrome/browser/browser_main.cc:412: #endif #endif // defined(OS_POSIX)
Thanks for pointing out PLOG to me, I didn't know that existed. http://codereview.chromium.org/460094/diff/3001/3002 File base/logging.cc (right): http://codereview.chromium.org/460094/diff/3001/3002#newcode693 base/logging.cc:693: void RawLog(int lvl, const char* message) { On 2009/12/07 20:29:39, Mark Mentovai wrote: > Again with lvl. Done. http://codereview.chromium.org/460094/diff/3001/3002#newcode696 base/logging.cc:696: const size_t buflen = strlen(message); On 2009/12/07 20:29:39, Mark Mentovai wrote: > buflen sounds like something that it's not, I'd call this message_len. Done. http://codereview.chromium.org/460094/diff/3001/3002#newcode709 base/logging.cc:709: if (buflen > 0 && message[buflen-1] != '\n') { On 2009/12/07 20:29:39, Mark Mentovai wrote: > Spaces around the - also. Done. http://codereview.chromium.org/460094/diff/3001/3004 File chrome/browser/browser_main.cc (right): http://codereview.chromium.org/460094/diff/3001/3004#newcode178 chrome/browser/browser_main.cc:178: RAW_CHECK(signal == expected_signal); On 2009/12/07 20:29:39, Mark Mentovai wrote: > I don't know if having expected_signal as an argument here makes much sense, > since it's only used for the CHECK. It seems about as useful to just move these > RAW_CHECKs to the SIG*Handlers below. Done. http://codereview.chromium.org/460094/diff/3001/3004#newcode189 chrome/browser/browser_main.cc:189: RAW_CHECK(HANDLE_EINTR(write(g_shutdown_pipe_write_fd, "\0", 1) == 1)); On 2009/12/07 20:29:39, Mark Mentovai wrote: > Maybe we want to write the signal number to the pipe so it's available on the > other end? Done. http://codereview.chromium.org/460094/diff/3001/3004#newcode246 chrome/browser/browser_main.cc:246: : shutdown_fd_(shutdown_fd) { On 2009/12/07 20:29:39, Mark Mentovai wrote: > 4-space indent on this line, not 2. Done. http://codereview.chromium.org/460094/diff/3001/3004#newcode259 chrome/browser/browser_main.cc:259: NOTREACHED() << "Unexpected error: " << strerror(errno); On 2009/12/07 20:29:39, Mark Mentovai wrote: > I guess there's no PNOTREACHED, huh? Nope. I'm happy to add one though. http://codereview.chromium.org/460094/diff/3001/3004#newcode402 chrome/browser/browser_main.cc:402: LOG(DFATAL) << "Failed to create pipe: " << strerror(errno); On 2009/12/07 20:29:39, Mark Mentovai wrote: > Use PLOG here, not LOG. PLOG does the strerror thing. Done. http://codereview.chromium.org/460094/diff/3001/3004#newcode410 chrome/browser/browser_main.cc:410: LOG(DFATAL) << "Failed to create shutdown detector task."; On 2009/12/07 20:29:39, Mark Mentovai wrote: > Wrap this in {}s, because you've got a multi-line conditional and it'll be > easier to read. > > The conditional is already kind of hard to read... Done. http://codereview.chromium.org/460094/diff/3001/3004#newcode412 chrome/browser/browser_main.cc:412: #endif On 2009/12/07 20:29:39, Mark Mentovai wrote: > #endif // defined(OS_POSIX) Done.
willchan@chromium.org wrote: > Thanks for pointing out PLOG to me, I didn't know that existed. [...] >> I guess there's no PNOTREACHED, huh? > > Nope. I'm happy to add one though. Up to you. Mark
http://codereview.chromium.org/460094/diff/17/18 File base/logging.cc (right): http://codereview.chromium.org/460094/diff/17/18#newcode698 base/logging.cc:698: do { Move "while (bytes_written < message_len) {" to the top instead of doing "do...while" to avoid calling write() at all in the unexpected and stupid case of message_len == 0. http://codereview.chromium.org/460094/diff/17/18#newcode702 base/logging.cc:702: if (rv < 0) { What if we wrote 0? Line 712 also. I only ask because in browser_main.cc, you're treating a 0 return from "write" differently than you're treating it here. http://codereview.chromium.org/460094/diff/17/19 File base/logging.h (right): http://codereview.chromium.org/460094/diff/17/19#newcode776 base/logging.h:776: void RawLog(int lvl, const char* message); lvl -> level (you fixed it in the other file, but not here.) http://codereview.chromium.org/460094/diff/17/20 File chrome/browser/browser_main.cc (right): http://codereview.chromium.org/460094/diff/17/20#newcode193 chrome/browser/browser_main.cc:193: RAW_CHECK(rv > 0); This is where you treat a 0 return value from "write" differently than in logging.cc. http://codereview.chromium.org/460094/diff/17/20#newcode269 chrome/browser/browser_main.cc:269: NOTREACHED() << "Unexpected error: " << strerror(errno); I think that both of these NOTREACHEDs should be followed by "break;" so that we'll fall out of the "read" loop and proceed with posting the CloseAllBrowsersAndExit task. NOTREACHED is a "D" check but we should handle it in non-"D" mode too. http://codereview.chromium.org/460094/diff/17/20#newcode276 chrome/browser/browser_main.cc:276: LOG(INFO) << "Handling shutdown for signal: " << signal << "."; Maybe it's just me, but the : in the string isn't helpful, and the message will read better without it. http://codereview.chromium.org/460094/diff/17/20#newcode278 chrome/browser/browser_main.cc:278: int rv = HANDLE_EINTR(close(shutdown_fd_)); I wonder why we'd want to do this. There may be handlers for other signals still installed. Shouldn't we leave shutdown_fd_ open and put all of ThreadMain inside a while (true) loop? It may not matter with the current implementation of CloseAllBrowsersAndExit, but I think it's the right way to go. The response to a signal may change in the future - maybe there'll be "do you really want to quit?" UI at some point even if that doesn't exist today, and taking distinct signals or even the same signal more than once during the lifetime of a process doesn't seem terribly implausible. http://codereview.chromium.org/460094/diff/17/21 File chrome/browser/chrome_thread.cc (right): http://codereview.chromium.org/460094/diff/17/21#newcode157 chrome/browser/chrome_thread.cc:157: DCHECK(identifier >= 0 && identifier < ID_COUNT); Since this is a only "D" check, and an out-of-bounds identifier could be bad but can easily be handled here, I'd leave this DCHECK in place but rewrite the next line as... http://codereview.chromium.org/460094/diff/17/21#newcode159 chrome/browser/chrome_thread.cc:159: return chrome_threads_[identifier] != NULL; return identifier >= 0 && identifier < ID_COUNT && chrome_threads_[identifier] != NULL;
http://codereview.chromium.org/460094/diff/17/18 File base/logging.cc (right): http://codereview.chromium.org/460094/diff/17/18#newcode698 base/logging.cc:698: do { On 2009/12/07 21:27:38, Mark Mentovai wrote: > Move "while (bytes_written < message_len) {" to the top instead of doing > "do...while" to avoid calling write() at all in the unexpected and stupid case > of message_len == 0. Done. http://codereview.chromium.org/460094/diff/17/18#newcode702 base/logging.cc:702: if (rv < 0) { On 2009/12/07 21:27:38, Mark Mentovai wrote: > What if we wrote 0? Line 712 also. I only ask because in browser_main.cc, > you're treating a 0 return from "write" differently than you're treating it > here. I've changed the browser_main.cc one to be consistent with here. This should handle the zero byte write case correctly. http://codereview.chromium.org/460094/diff/17/19 File base/logging.h (right): http://codereview.chromium.org/460094/diff/17/19#newcode776 base/logging.h:776: void RawLog(int lvl, const char* message); On 2009/12/07 21:27:38, Mark Mentovai wrote: > lvl -> level (you fixed it in the other file, but not here.) Done. http://codereview.chromium.org/460094/diff/17/20 File chrome/browser/browser_main.cc (right): http://codereview.chromium.org/460094/diff/17/20#newcode193 chrome/browser/browser_main.cc:193: RAW_CHECK(rv > 0); On 2009/12/07 21:27:38, Mark Mentovai wrote: > This is where you treat a 0 return value from "write" differently than in > logging.cc. Changed to match logging.cc. http://codereview.chromium.org/460094/diff/17/20#newcode269 chrome/browser/browser_main.cc:269: NOTREACHED() << "Unexpected error: " << strerror(errno); On 2009/12/07 21:27:38, Mark Mentovai wrote: > I think that both of these NOTREACHEDs should be followed by "break;" so that > we'll fall out of the "read" loop and proceed with posting the > CloseAllBrowsersAndExit task. NOTREACHED is a "D" check but we should handle it > in non-"D" mode too. Done. http://codereview.chromium.org/460094/diff/17/20#newcode276 chrome/browser/browser_main.cc:276: LOG(INFO) << "Handling shutdown for signal: " << signal << "."; On 2009/12/07 21:27:38, Mark Mentovai wrote: > Maybe it's just me, but the : in the string isn't helpful, and the message will > read better without it. Done. http://codereview.chromium.org/460094/diff/17/20#newcode278 chrome/browser/browser_main.cc:278: int rv = HANDLE_EINTR(close(shutdown_fd_)); On 2009/12/07 21:27:38, Mark Mentovai wrote: > I wonder why we'd want to do this. There may be handlers for other signals > still installed. Shouldn't we leave shutdown_fd_ open and put all of ThreadMain > inside a while (true) loop? It may not matter with the current implementation > of CloseAllBrowsersAndExit, but I think it's the right way to go. The response > to a signal may change in the future - maybe there'll be "do you really want to > quit?" UI at some point even if that doesn't exist today, and taking distinct > signals or even the same signal more than once during the lifetime of a process > doesn't seem terribly implausible. I think this case is implausible for the shutdown pipe, since it's specifically for shutdown, so I'm fine with ignoring the other signals after the pipe has already been written to, since CloseAllBrowsersAndExit() is just going to shut it down anyway. I've removed the close() so that we don't get write() failures on subsequent distinct signals. I haven't done the while loop since I think it's misleading to have that code in there right now, since no one has implemented your hypothetical UI, and it would require adding a valgrind suppression for the leaked thread. If we need the UI, then we can make the change at that point. http://codereview.chromium.org/460094/diff/17/21 File chrome/browser/chrome_thread.cc (right): http://codereview.chromium.org/460094/diff/17/21#newcode159 chrome/browser/chrome_thread.cc:159: return chrome_threads_[identifier] != NULL; On 2009/12/07 21:27:38, Mark Mentovai wrote: > return identifier >= 0 && identifier < ID_COUNT && chrome_threads_[identifier] > != NULL; Done.
Nice. LGTM now. Thanks for fixing this. http://codereview.chromium.org/460094/diff/5003/3013 File base/logging.cc (right): http://codereview.chromium.org/460094/diff/5003/3013#newcode707 base/logging.cc:707: }; ; unnecessary, remove it.
http://codereview.chromium.org/460094/diff/26/29 File chrome/browser/browser_main.cc (right): http://codereview.chromium.org/460094/diff/26/29#newcode184 chrome/browser/browser_main.cc:184: if (ChromeThread::HasThread(ChromeThread::UI)) { Is there no other way to solve it except by adding a new function to ChromeThred? When will it ever happen that HasThread(ChromeThread::UI) returns false?
http://codereview.chromium.org/460094/diff/26/29 File chrome/browser/browser_main.cc (right): http://codereview.chromium.org/460094/diff/26/29#newcode184 chrome/browser/browser_main.cc:184: if (ChromeThread::HasThread(ChromeThread::UI)) { On 2009/12/07 23:40:25, John Abd-El-Malek wrote: > Is there no other way to solve it except by adding a new function to > ChromeThred? When will it ever happen that HasThread(ChromeThread::UI) returns > false? I've moved the creation of the shutdown thread to after the UI thread is registered to remove this condition. I've moved the error handling code into the shutdown thread function. Mark, let me know if this is ok with you.
I think the most recent changes are fine. LGTM still.
http://codereview.chromium.org/460094/diff/26/29 File chrome/browser/browser_main.cc (right): http://codereview.chromium.org/460094/diff/26/29#newcode184 chrome/browser/browser_main.cc:184: if (ChromeThread::HasThread(ChromeThread::UI)) { On 2009/12/08 03:16:11, willchan wrote: > On 2009/12/07 23:40:25, John Abd-El-Malek wrote: > > Is there no other way to solve it except by adding a new function to > > ChromeThred? When will it ever happen that HasThread(ChromeThread::UI) > returns > > false? > > I've moved the creation of the shutdown thread to after the UI thread is > registered to remove this condition. I've moved the error handling code into > the shutdown thread function. Mark, let me know if this is ok with you. great, thanks
willchan, Sorry for my stupid comment in advance. One of our Linux buildbots has been reporting a memory leak since Build 2213, which includes this change only. <http://build.chromium.org/buildbot/waterfall/builders/Linux%20Tests%20(valgri...> Is it possible to investigate why your change caused this memory leak? Regards, Hironori Bono
http://codereview.chromium.org/460094/diff/5005/3020 File chrome/browser/browser_main.cc (right): http://codereview.chromium.org/460094/diff/5005/3020#newcode432 chrome/browser/browser_main.cc:432: new ShutdownDetector(g_shutdown_pipe_read_fd))) { I'm wondering when this ShutdownDetector instance is deleted. (Is this caused the memory leak in one of our Linux buildbots since Build 2213?) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
