[patches] Re: [commits] [tarantool] 01/01: log: Fix log rotation is not async signal safe

Georgy Kirichenko georgy at tarantool.org
Thu Feb 15 13:35:21 MSK 2018


I think SIGHUP signal handler should just send an event to ev_loop. 
Corresponding event handler will do all job and can be safelly called from 
other places (both calls in this case will be done synchronously without 
sideeffects).
Current log_rotate is synchronous too, because a log_rotate stuck will freeze 
the whole application even at signal handler. But it should be nice to do log 
rotation through coio task.

On Tuesday, January 30, 2018 10:47:53 PM MSK Konstantin Osipov wrote:
> * IlyaMarkovMipt <markovilya197 at gmail.com> [18/01/24 13:34]:
> > commit a254d2b1cd8fb316ac3ad3ae36d59686d86cc8e8
> > Author: IlyaMarkovMipt <markovilya197 at gmail.com>
> > AuthorDate: Tue Jan 23 16:23:29 2018 +0300
> > 
> >     log: Fix log rotation is not async signal safe
> >     
> >     * Refactor signal handling with ev_signal.
> >     * Add another signal watcher in main.
> >     
> >     We claim multiple loggers non-thread-safe,
> >     so concurrency in logrotate signal handler
> >     should not be considered.
> 
> I'd be very cautious to make log rotation non-asynchronous.
> If we run out of disk space, and the logger goes to a file,
> write() can be completely stuck until there is available disk
> space. In this case the entire event loop will be stuck, and
> the system will sit in a dead lock.
> 
> >     Relates #3015
> > 
> > ---
> > 
> >  src/lua/log.lua | 8 ++++++--
> >  src/main.cc     | 3 ++-
> >  src/say.c       | 7 ++++---
> >  src/say.h       | 5 ++++-
> >  4 files changed, 16 insertions(+), 7 deletions(-)
> > 
> > diff --git a/src/lua/log.lua b/src/lua/log.lua
> > index b90eb88..0ac0e8f 100644
> > --- a/src/lua/log.lua
> > +++ b/src/lua/log.lua
> > @@ -12,7 +12,11 @@ ffi.cdef[[
> > 
> >      extern sayfunc_t _say;
> > 
> > -    extern void say_logrotate(int);
> > +    extern struct ev_loop;
> > +    extern struct ev_signal;
> > +
> > +    extern void
> > +    say_logrotate(struct ev_loop *, struct ev_signal *, int);
> > 
> >      enum say_level {
> >      
> >          S_FATAL,
> > 
> > @@ -104,7 +108,7 @@ local function say_closure(lvl)
> > 
> >  end
> >  
> >  local function log_rotate()
> > 
> > -    ffi.C.say_logrotate(0)
> > +    ffi.C.say_logrotate(nil, nil, 0)
> > 
> >  end
> >  
> >  local function log_level(level)
> > 
> > diff --git a/src/main.cc b/src/main.cc
> > index ec06103..1682bae 100644
> > --- a/src/main.cc
> > +++ b/src/main.cc
> > @@ -83,7 +83,7 @@ static char *pid_file = NULL;
> > 
> >  static char **main_argv;
> >  static int main_argc;
> >  /** Signals handled after start as part of the event loop. */
> > 
> > -static ev_signal ev_sigs[5];
> > +static ev_signal ev_sigs[6];
> > 
> >  static const int ev_sig_count = sizeof(ev_sigs)/sizeof(*ev_sigs);
> >  
> >  static double start_time;
> > 
> > @@ -335,6 +335,7 @@ signal_init(void)
> > 
> >  	ev_signal_init(&ev_sigs[2], signal_cb, SIGTERM);
> >  	ev_signal_init(&ev_sigs[3], signal_cb, SIGHUP);
> >  	ev_signal_init(&ev_sigs[4], signal_sigwinch_cb, SIGWINCH);
> > 
> > +	ev_signal_init(&ev_sigs[5], say_logrotate, SIGHUP);
> > 
> >  	for (int i = 0; i < ev_sig_count; i++)
> >  	
> >  		ev_signal_start(loop(), &ev_sigs[i]);
> > 
> > diff --git a/src/say.c b/src/say.c
> > index 65c1872..394e6b3 100644
> > --- a/src/say.c
> > +++ b/src/say.c
> > @@ -269,9 +269,11 @@ log_rotate(const struct log *log)
> > 
> >  }
> >  
> >  void
> > 
> > -say_logrotate(int signo)
> > +say_logrotate(struct ev_loop *loop, struct ev_signal *w, int revents)
> > 
> >  {
> > 
> > -	(void) signo;
> > +	(void) loop;
> > +	(void) w;
> > +	(void) revents;
> > 
> >  	int saved_errno = errno;
> >  	struct log *log;
> >  	rlist_foreach_entry(log, &log_rotate_list, in_log_list) {
> > 
> > @@ -535,7 +537,6 @@ say_logger_init(const char *init_str, int level, int
> > nonblock,> 
> >  	say_set_log_level(level);
> >  	log_background = background;
> >  	log_pid = log_default->pid;
> > 
> > -	signal(SIGHUP, say_logrotate);
> > 
> >  	say_set_log_format(say_format_by_name(format));
> >  	
> >  	if (background) {
> > 
> > diff --git a/src/say.h b/src/say.h
> > index 1b63229..46e6976 100644
> > --- a/src/say.h
> > +++ b/src/say.h
> > @@ -185,8 +185,11 @@ say_set_log_format(enum say_format format);
> > 
> >  enum say_format
> >  say_format_by_name(const char *format);
> > 
> > +struct ev_loop;
> > +struct ev_signal;
> > +
> > 
> >  void
> > 
> > -say_logrotate(int /* signo */);
> > +say_logrotate(struct ev_loop *, struct ev_signal *, int /* revents */);
> > 
> >  /** Init default logger. */
> >  void







More information about the Tarantool-patches mailing list