Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10905081:
Pass Me2Me config via stdin. (Closed)
|
Created:
8 years, 3 months ago by Jamie Modified:
8 years, 3 months ago CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, simonmorris+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, rmsousa+watch_chromium.org, pam+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPass Me2Me config via stdin.
This allows the host to restart even if the config file is no longer readable (for example, if it is on an NFS share). One downside is that the host can no longer re-read its configuration without restarting.
I ended up implementing a SIGTERM handler to provide clean shutdown. It's easily extractable into a separate CL; let me know if you'd like me to do that.
BUG=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=155314
Patch Set 1 #Patch Set 2 : Delete debugging and clean up comments. #
Total comments: 15
Patch Set 3 : Reviewer comments. #Patch Set 4 : Rebase #Patch Set 5 : Fixed #include guard footer. #
Total comments: 25
Patch Set 6 : Improved variable names. #
Total comments: 7
Patch Set 7 : 7739 #
Total comments: 3
Patch Set 8 : Removed (probably) unnecessary clean-up code. Improved doc comment. #Patch Set 9 : Reinstate and improve error handling. #
Messages
Total messages: 23 (0 generated)
ptal
On 2012/09/04 21:40:13, Jamie wrote: > ptal Could you please wait checking it in until https://chromiumcodereview.appspot.com/10855249/ (LGTMed) is in? It has changes that this CL likely wants to depend on.
Please don't take my accidental L G T M above as actual L G T M. :-)
https://chromiumcodereview.appspot.com/10905081/diff/2001/remoting/host/remot... File remoting/host/remoting_me2me_host.cc (right): https://chromiumcodereview.appspot.com/10905081/diff/2001/remoting/host/remot... remoting/host/remoting_me2me_host.cc:166: void SignalHandler(int signum) { Why not separate SighupHandler() and SigtermHandler() given that RegisterSignalHandler() supports it? https://chromiumcodereview.appspot.com/10905081/diff/2001/remoting/tools/me2m... File remoting/tools/me2me_virtual_host.py (right): https://chromiumcodereview.appspot.com/10905081/diff/2001/remoting/tools/me2m... remoting/tools/me2me_virtual_host.py:536: def reload_config(self): This method is not used outside of this class, so it should be private. call it _reload_config()?
lgtm with comments http://codereview.chromium.org/10905081/diff/2001/remoting/host/posix/signal_... File remoting/host/posix/signal_handler.cc (right): http://codereview.chromium.org/10905081/diff/2001/remoting/host/posix/signal_... remoting/host/posix/signal_handler.cc:9: #include <list> nit: C++ after C headers And #include <utility> for std::pair http://codereview.chromium.org/10905081/diff/2001/remoting/host/posix/signal_... remoting/host/posix/signal_handler.cc:38: typedef std::list<SignalAndHandler> SignalHandlers; std::multimap would make the code slightly shorter - maybe not enough benefit to be worth changing at this point? http://codereview.chromium.org/10905081/diff/2001/remoting/host/posix/signal_... File remoting/host/posix/signal_handler.h (right): http://codereview.chromium.org/10905081/diff/2001/remoting/host/posix/signal_... remoting/host/posix/signal_handler.h:9: #ifndef REMOTING_HOST_POSIX_SIGHUP_LISTENER_H_ nit: match filename http://codereview.chromium.org/10905081/diff/2001/remoting/host/remoting_me2m... File remoting/host/remoting_me2me_host.cc (right): http://codereview.chromium.org/10905081/diff/2001/remoting/host/remoting_me2m... remoting/host/remoting_me2me_host.cc:57: #include <signal.h> nit: move system include above project include http://codereview.chromium.org/10905081/diff/2001/remoting/host/remoting_me2m... remoting/host/remoting_me2me_host.cc:175: &HostProcess::Shutdown, base::Unretained(this), kSuccessExitCode)); Could you comment why base::Unretained is safe here? Is there a possibility of nastiness if, say, this process is sent SIGTERM immediately followed by SIGHUP? Or maybe we don't need to worry? http://codereview.chromium.org/10905081/diff/2001/remoting/tools/me2me_virtua... File remoting/tools/me2me_virtual_host.py (right): http://codereview.chromium.org/10905081/diff/2001/remoting/tools/me2me_virtua... remoting/tools/me2me_virtual_host.py:662: print >> sys.stderr, "Failed to load " + config_filename logging.error() is preferred for printing error messages. We should eventually fix other instances to be more consistent. Now I'm thinking about it, we really should be using logging.fatal() whenever the error is serious enough to exit the program. For this CL, I think "logging.error()" is OK for now, but do whatever you think's best.
Alex, ptal to make sure this still makes sense after rebasing to your CL. In particular, I've removed the SIGHUP handler. https://chromiumcodereview.appspot.com/10905081/diff/2001/remoting/host/posix... File remoting/host/posix/signal_handler.cc (right): https://chromiumcodereview.appspot.com/10905081/diff/2001/remoting/host/posix... remoting/host/posix/signal_handler.cc:9: #include <list> On 2012/09/05 16:35:54, Lambros wrote: > nit: C++ after C headers > And #include <utility> for std::pair Done. https://chromiumcodereview.appspot.com/10905081/diff/2001/remoting/host/posix... remoting/host/posix/signal_handler.cc:38: typedef std::list<SignalAndHandler> SignalHandlers; On 2012/09/05 16:35:54, Lambros wrote: > std::multimap would make the code slightly shorter - maybe not enough benefit to > be worth changing at this point? I think I'll stick with list. multimap is not widely used and I think the code is clear enough using list. https://chromiumcodereview.appspot.com/10905081/diff/2001/remoting/host/posix... File remoting/host/posix/signal_handler.h (right): https://chromiumcodereview.appspot.com/10905081/diff/2001/remoting/host/posix... remoting/host/posix/signal_handler.h:9: #ifndef REMOTING_HOST_POSIX_SIGHUP_LISTENER_H_ On 2012/09/05 16:35:54, Lambros wrote: > nit: match filename Done. https://chromiumcodereview.appspot.com/10905081/diff/2001/remoting/host/remot... File remoting/host/remoting_me2me_host.cc (right): https://chromiumcodereview.appspot.com/10905081/diff/2001/remoting/host/remot... remoting/host/remoting_me2me_host.cc:57: #include <signal.h> On 2012/09/05 16:35:54, Lambros wrote: > nit: move system include above project include Done. https://chromiumcodereview.appspot.com/10905081/diff/2001/remoting/host/remot... remoting/host/remoting_me2me_host.cc:166: void SignalHandler(int signum) { On 2012/09/04 22:01:48, sergeyu wrote: > Why not separate SighupHandler() and SigtermHandler() given that > RegisterSignalHandler() supports it? Done. https://chromiumcodereview.appspot.com/10905081/diff/2001/remoting/host/remot... remoting/host/remoting_me2me_host.cc:175: &HostProcess::Shutdown, base::Unretained(this), kSuccessExitCode)); On 2012/09/05 16:35:54, Lambros wrote: > Could you comment why base::Unretained is safe here? Is there a possibility of > nastiness if, say, this process is sent SIGTERM immediately followed by SIGHUP? > Or maybe we don't need to worry? Done. https://chromiumcodereview.appspot.com/10905081/diff/2001/remoting/tools/me2m... File remoting/tools/me2me_virtual_host.py (right): https://chromiumcodereview.appspot.com/10905081/diff/2001/remoting/tools/me2m... remoting/tools/me2me_virtual_host.py:536: def reload_config(self): On 2012/09/04 22:01:48, sergeyu wrote: > This method is not used outside of this class, so it should be private. call it > _reload_config()? I just got rid of the function. There was no real need for it to be factored out.
It looks like there are a few issues in the CL. https://chromiumcodereview.appspot.com/10905081/diff/6004/remoting/host/posix... File remoting/host/posix/signal_handler.h (right): https://chromiumcodereview.appspot.com/10905081/diff/6004/remoting/host/posix... remoting/host/posix/signal_handler.h:18: // Register for SIGHUP notifications on the current thread, which must have nit: SIGHUP -> |signum| https://chromiumcodereview.appspot.com/10905081/diff/6004/remoting/host/remot... File remoting/host/remoting_me2me_host.cc (right): https://chromiumcodereview.appspot.com/10905081/diff/6004/remoting/host/remot... remoting/host/remoting_me2me_host.cc:58: #include <signal.h> This one is needed on Mac only. https://chromiumcodereview.appspot.com/10905081/diff/6004/remoting/host/remot... remoting/host/remoting_me2me_host.cc:59: #include "remoting/host/posix/signal_handler.h" This is used only on Linux. https://chromiumcodereview.appspot.com/10905081/diff/6004/remoting/host/remot... remoting/host/remoting_me2me_host.cc:160: void SigTermHandler(int signum) { This will blow up, if SIGTERM is received when shutting down HostProcess. https://chromiumcodereview.appspot.com/10905081/diff/6004/remoting/host/remot... remoting/host/remoting_me2me_host.cc:163: // base::Unretained is safe here because |this| is owned by the thread nit: This comment is not really true. The network thread does not own HostProcess instance, - the main thread does. Posting HostProcess::Shutdown() on the network thread is undeed the only supported way of shutting down HostProcess(). https://chromiumcodereview.appspot.com/10905081/diff/6004/remoting/host/remot... remoting/host/remoting_me2me_host.cc:234: #if defined(OS_POSIX) Shouldn't it be #if defined(OS_MAC)? https://chromiumcodereview.appspot.com/10905081/diff/6004/remoting/host/remot... remoting/host/remoting_me2me_host.cc:236: // |ConfigFileWatcher| on Mac and read the config from stdin on UNIX. nit: ...on Mac and Windows... https://chromiumcodereview.appspot.com/10905081/diff/6004/remoting/host/remot... remoting/host/remoting_me2me_host.cc:306: context_->file_task_runner()->PostTask( Why are we doing it in ShutdownHostProcess()? context_.reset() below will kill the file thread. Why should ListenForShutdownSignal be done on the file thread?
Also I don't see where OnConfigUpdated is called after the config is read from stdin.
OnConfigUpdated is being called, presumably by virtue of the fact that /dev/stdin behaves like a file on Linux. https://chromiumcodereview.appspot.com/10905081/diff/6004/remoting/host/posix... File remoting/host/posix/signal_handler.h (right): https://chromiumcodereview.appspot.com/10905081/diff/6004/remoting/host/posix... remoting/host/posix/signal_handler.h:18: // Register for SIGHUP notifications on the current thread, which must have On 2012/09/05 21:41:25, alexeypa wrote: > nit: SIGHUP -> |signum| Done. https://chromiumcodereview.appspot.com/10905081/diff/6004/remoting/host/remot... File remoting/host/remoting_me2me_host.cc (right): https://chromiumcodereview.appspot.com/10905081/diff/6004/remoting/host/remot... remoting/host/remoting_me2me_host.cc:58: #include <signal.h> On 2012/09/05 21:41:25, alexeypa wrote: > This one is needed on Mac only. I don't think that's true. <signal.h> is needed for SIGTERM, which we register for on both Mac and UNIX. https://chromiumcodereview.appspot.com/10905081/diff/6004/remoting/host/remot... remoting/host/remoting_me2me_host.cc:59: #include "remoting/host/posix/signal_handler.h" On 2012/09/05 21:41:25, alexeypa wrote: > This is used only on Linux. Ditto. https://chromiumcodereview.appspot.com/10905081/diff/6004/remoting/host/remot... remoting/host/remoting_me2me_host.cc:160: void SigTermHandler(int signum) { On 2012/09/05 21:41:25, alexeypa wrote: > This will blow up, if SIGTERM is received when shutting down HostProcess. As of the latest revision, this will be called on the network thread, which means that it no longer needs to post the shutdown task. https://chromiumcodereview.appspot.com/10905081/diff/6004/remoting/host/remot... remoting/host/remoting_me2me_host.cc:163: // base::Unretained is safe here because |this| is owned by the thread On 2012/09/05 21:41:25, alexeypa wrote: > nit: This comment is not really true. The network thread does not own > HostProcess instance, - the main thread does. Posting HostProcess::Shutdown() on > the network thread is undeed the only supported way of shutting down > HostProcess(). This is no longer an issue with the latest revision. https://chromiumcodereview.appspot.com/10905081/diff/6004/remoting/host/remot... remoting/host/remoting_me2me_host.cc:234: #if defined(OS_POSIX) On 2012/09/05 21:41:25, alexeypa wrote: > Shouldn't it be #if defined(OS_MAC)? Arguably, it isn't needed on either platform since we shouldn't be sending SIGHUP to these processes. I don't think it's common practise to ignore signals you're not expecting to receive. https://chromiumcodereview.appspot.com/10905081/diff/6004/remoting/host/remot... remoting/host/remoting_me2me_host.cc:236: // |ConfigFileWatcher| on Mac and read the config from stdin on UNIX. On 2012/09/05 21:41:25, alexeypa wrote: > nit: ...on Mac and Windows... Done. https://chromiumcodereview.appspot.com/10905081/diff/6004/remoting/host/remot... remoting/host/remoting_me2me_host.cc:306: context_->file_task_runner()->PostTask( On 2012/09/05 21:41:25, alexeypa wrote: > Shouldn't it be #if defined(OS_MAC)? Arguably, it isn't needed on either platform since we shouldn't be sending SIGHUP to these processes. I don't think it's common practise to ignore signals you're not expecting to receive. In any case, we shouldn't be treating Mac differently to UNIX.
Couple of naming nits I missed first time round :) http://codereview.chromium.org/10905081/diff/6004/remoting/host/posix/signal_... File remoting/host/posix/signal_handler.cc (right): http://codereview.chromium.org/10905081/diff/6004/remoting/host/posix/signal_... remoting/host/posix/signal_handler.cc:82: bool RegisterSignalHandler(int signum, const SignalHandler& handler) { nit: signal or signal_number ? http://codereview.chromium.org/10905081/diff/6004/remoting/host/posix/signal_... remoting/host/posix/signal_handler.cc:87: int pipefd[2]; nit: pipe_fd seems better ? http://codereview.chromium.org/10905081/diff/6004/remoting/host/posix/signal_... File remoting/host/posix/signal_handler.h (right): http://codereview.chromium.org/10905081/diff/6004/remoting/host/posix/signal_... remoting/host/posix/signal_handler.h:20: bool RegisterSignalHandler(int signum, const SignalHandler& handler); nit: signal or signal_number perhaps? http://codereview.chromium.org/10905081/diff/6004/remoting/host/remoting_me2m... File remoting/host/remoting_me2me_host.cc (right): http://codereview.chromium.org/10905081/diff/6004/remoting/host/remoting_me2m... remoting/host/remoting_me2me_host.cc:160: void SigTermHandler(int signum) { nit: signal or signal_number ?
Alex, do you still have concerns about this? https://chromiumcodereview.appspot.com/10905081/diff/6004/remoting/host/posix... File remoting/host/posix/signal_handler.cc (right): https://chromiumcodereview.appspot.com/10905081/diff/6004/remoting/host/posix... remoting/host/posix/signal_handler.cc:82: bool RegisterSignalHandler(int signum, const SignalHandler& handler) { On 2012/09/06 00:47:19, Lambros wrote: > nit: signal or signal_number ? Done. https://chromiumcodereview.appspot.com/10905081/diff/6004/remoting/host/posix... remoting/host/posix/signal_handler.cc:87: int pipefd[2]; On 2012/09/06 00:47:19, Lambros wrote: > nit: pipe_fd seems better ? Done. https://chromiumcodereview.appspot.com/10905081/diff/6004/remoting/host/remot... File remoting/host/remoting_me2me_host.cc (right): https://chromiumcodereview.appspot.com/10905081/diff/6004/remoting/host/remot... remoting/host/remoting_me2me_host.cc:160: void SigTermHandler(int signum) { On 2012/09/06 00:47:19, Lambros wrote: > nit: signal or signal_number ? Done.
lgtm with nits. https://chromiumcodereview.appspot.com/10905081/diff/6004/remoting/host/remot... File remoting/host/remoting_me2me_host.cc (right): https://chromiumcodereview.appspot.com/10905081/diff/6004/remoting/host/remot... remoting/host/remoting_me2me_host.cc:234: #if defined(OS_POSIX) On 2012/09/06 00:18:50, Jamie wrote: > On 2012/09/05 21:41:25, alexeypa wrote: > > Shouldn't it be #if defined(OS_MAC)? > > Arguably, it isn't needed on either platform since we shouldn't be sending > SIGHUP to these processes. I don't think it's common practise to ignore signals > you're not expecting to receive. As per our discussion: either remove this code if SIGHUP is not sent anymore or follow up with another CL that removes SIGHUP sending code (and this code). https://chromiumcodereview.appspot.com/10905081/diff/8005/remoting/host/posix... File remoting/host/posix/signal_handler.cc (right): https://chromiumcodereview.appspot.com/10905081/diff/8005/remoting/host/posix... remoting/host/posix/signal_handler.cc:85: g_signal_listener = new SignalListener(); nit: This is not thread safe. Consider using lazy initialization. https://chromiumcodereview.appspot.com/10905081/diff/8005/remoting/host/posix... remoting/host/posix/signal_handler.cc:97: if (!result) { nit: Is this a sign of something going very wrong? Should it CHECK instead? https://chromiumcodereview.appspot.com/10905081/diff/8005/remoting/host/posix... remoting/host/posix/signal_handler.cc:99: g_signal_listener = NULL; nit: If WatchFileDescriptor() failed the first time, will likely it fail again? What is the point of retrying in this case? https://chromiumcodereview.appspot.com/10905081/diff/8005/remoting/host/remot... File remoting/host/remoting_me2me_host.cc (right): https://chromiumcodereview.appspot.com/10905081/diff/8005/remoting/host/remot... remoting/host/remoting_me2me_host.cc:292: base::Bind(&HostProcess::ListenForShutdownSignal, nit: You can bind to remoting::RegisterSignalHandler directly here.
https://chromiumcodereview.appspot.com/10905081/diff/6004/remoting/host/remot... File remoting/host/remoting_me2me_host.cc (right): https://chromiumcodereview.appspot.com/10905081/diff/6004/remoting/host/remot... remoting/host/remoting_me2me_host.cc:234: #if defined(OS_POSIX) On 2012/09/06 18:21:09, alexeypa wrote: > On 2012/09/06 00:18:50, Jamie wrote: > > On 2012/09/05 21:41:25, alexeypa wrote: > > > Shouldn't it be #if defined(OS_MAC)? > > > > Arguably, it isn't needed on either platform since we shouldn't be sending > > SIGHUP to these processes. I don't think it's common practise to ignore > signals > > you're not expecting to receive. > > As per our discussion: either remove this code if SIGHUP is not sent anymore or > follow up with another CL that removes SIGHUP sending code (and this code). SIGHUP will never be sent to the host process by any of our code (with the exception of the Mac launchd script, which forwards all signals so it doesn't count). I've removed this block.
fyi https://chromiumcodereview.appspot.com/10905081/diff/8005/remoting/host/posix... File remoting/host/posix/signal_handler.cc (right): https://chromiumcodereview.appspot.com/10905081/diff/8005/remoting/host/posix... remoting/host/posix/signal_handler.cc:85: g_signal_listener = new SignalListener(); On 2012/09/06 18:21:10, alexeypa wrote: > nit: This is not thread safe. Consider using lazy initialization. I've clarified that all calls to this function must be made on the same thread, otherwise the guarantee of being called back on the same thread that you registered on is not honoured. https://chromiumcodereview.appspot.com/10905081/diff/8005/remoting/host/posix... remoting/host/posix/signal_handler.cc:97: if (!result) { On 2012/09/06 18:21:10, alexeypa wrote: > nit: Is this a sign of something going very wrong? Should it CHECK instead? TBH, I'm not sure under what circumstances it can fail. CHECK doesn't feel right, since the caller may be able to proceed without signal handling. In our use-case, for example, we lose clean shutdown, but that's all. https://chromiumcodereview.appspot.com/10905081/diff/8005/remoting/host/posix... remoting/host/posix/signal_handler.cc:99: g_signal_listener = NULL; On 2012/09/06 18:21:10, alexeypa wrote: > nit: If WatchFileDescriptor() failed the first time, will likely it fail again? > What is the point of retrying in this case? Probably no point. I'll get rid of this clean-up code.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/10905081/15003
https://chromiumcodereview.appspot.com/10905081/diff/2005/remoting/host/posix... File remoting/host/posix/signal_handler.cc (right): https://chromiumcodereview.appspot.com/10905081/diff/2005/remoting/host/posix... remoting/host/posix/signal_handler.cc:99: g_signal_listener = NULL; One problem with this is that the 2nd call will succeed adding a handler to |g_signal_listener|. The handler will never be called though. The OS signal will be handled successfully leaving bytes in the pipe's buffer. I think it is better to guarantee in some way that if the 1st call failed then the other calls will fail too.
ptal https://chromiumcodereview.appspot.com/10905081/diff/2005/remoting/host/posix... File remoting/host/posix/signal_handler.cc (right): https://chromiumcodereview.appspot.com/10905081/diff/2005/remoting/host/posix... remoting/host/posix/signal_handler.cc:99: g_signal_listener = NULL; On 2012/09/06 19:20:14, alexeypa wrote: > One problem with this is that the 2nd call will succeed adding a handler to > |g_signal_listener|. The handler will never be called though. The OS signal will > be handled successfully leaving bytes in the pipe's buffer. > > I think it is better to guarantee in some way that if the 1st call failed then > the other calls will fail too. That's a good point. I think I'm going to go back on my previous decision and reinstate the error handling, improved a bit to cope with failure of the |pipe| API. If the first registration fails, it leaves things in a state where it's safe to retry (though I agree that it probably won't work the second time either).
lgtm https://chromiumcodereview.appspot.com/10905081/diff/2005/remoting/host/posix... File remoting/host/posix/signal_handler.cc (right): https://chromiumcodereview.appspot.com/10905081/diff/2005/remoting/host/posix... remoting/host/posix/signal_handler.cc:99: g_signal_listener = NULL; On 2012/09/06 21:33:59, Jamie wrote: > On 2012/09/06 19:20:14, alexeypa wrote: > > One problem with this is that the 2nd call will succeed adding a handler to > > |g_signal_listener|. The handler will never be called though. The OS signal > will > > be handled successfully leaving bytes in the pipe's buffer. > > > > I think it is better to guarantee in some way that if the 1st call failed then > > the other calls will fail too. > > That's a good point. I think I'm going to go back on my previous decision and > reinstate the error handling, improved a bit to cope with failure of the |pipe| > API. If the first registration fails, it leaves things in a state where it's > safe to retry (though I agree that it probably won't work the second time > either). Sounds good to me.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/10905081/4007
Try job failure for 10905081-4007 (retry) on win_rel for step "update". It's a second try, previously, step "runhooks" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu... Step "update" is always a major failure. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/10905081/4007
Change committed as 155314 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
