From cc52a59e1ac7155123a314067cc8ad9f3affe491 Mon Sep 17 00:00:00 2001 From: Kevin Ballard Date: Sun, 24 Aug 2014 00:59:03 -0700 Subject: [PATCH] Rework how screen size is tracked The screen size is fetched after a SIGWINCH is delivered. The current implementation has two issues: * It calls ioctl() from the SIGWINCH signal handler, despite ioctl() not being a function that is known to be safe to call. * It's not thread-safe. Signals can be delivered on arbitrary threads, so we don't know if it's actually safe to be modifying the cached winsize in response to a signal. It's also plausible that the winsize may be requested from a background thread. To solve the first issue, we twiddle a volatile boolean flag in the signal handler and defer the ioctl() call until we actually request the screen size. To solve the second issue, we introduce a pthread rwlock around the cached winsize. A rwlock is used because it can be expected that there are likely to be far more window size reads than window size writes. If we were using C++11 we could probably get away with atomics, but since we don't have that (or boost), a rwlock should suffice. Fixes #1613. --- common.cpp | 125 +++++++++++++++++++++++++++++++++++++++++++++------ common.h | 50 +++++++++++++++++++-- function.cpp | 3 -- 3 files changed, 159 insertions(+), 19 deletions(-) diff --git a/common.cpp b/common.cpp index b8a2661d2..ad35470e5 100644 --- a/common.cpp +++ b/common.cpp @@ -90,9 +90,12 @@ const wchar_t *program_name; int debug_level=1; /** - This struct should be continually updated by signals as the term resizes, and as such always contain the correct current size. + This struct maintains the current state of the terminal size. It is updated on demand after receiving a SIGWINCH. + Do not touch this struct directly, it's managed with a rwlock. Use common_get_width()/common_get_height(). */ static struct winsize termsize; +static volatile bool termsize_valid; +static rwlock_t termsize_rwlock; static char *wcs2str_internal(const wchar_t *in, char *out); @@ -1686,26 +1689,44 @@ bool unescape_string(const wcstring &input, wcstring *output, unescape_flags_t e void common_handle_winch(int signal) { -#ifdef HAVE_WINSIZE - if (ioctl(1,TIOCGWINSZ,&termsize)!=0) - { - return; - } -#else - termsize.ws_col = 80; - termsize.ws_row = 24; + /* don't run ioctl() here, it's not safe to use in signals */ + termsize_valid = false; +} + +/* updates termsize as needed, and returns a copy of the winsize. */ +static struct winsize get_current_winsize() +{ +#ifndef HAVE_WINSIZE + struct winsize retval = {0}; + retval.ws_col = 80; + retval.ws_row = 24; + return retval; #endif + scoped_rwlock guard(termsize_rwlock, true); + struct winsize retval = termsize; + if (!termsize_valid) + { + struct winsize size; + if (ioctl(1,TIOCGWINSZ,&size) == 0) + { + retval = size; + guard.upgrade(); + termsize = retval; + } + termsize_valid = true; + } + return retval; } int common_get_width() { - return termsize.ws_col; + return get_current_winsize().ws_col; } int common_get_height() { - return termsize.ws_row; + return get_current_winsize().ws_row; } void tokenize_variable_array(const wcstring &val, std::vector &out) @@ -2223,7 +2244,7 @@ void scoped_lock::lock(void) { assert(! locked); assert(! is_forked_child()); - VOMIT_ON_FAILURE(pthread_mutex_lock(lock_obj)); + VOMIT_ON_FAILURE_NO_ERRNO(pthread_mutex_lock(lock_obj)); locked = true; } @@ -2231,7 +2252,7 @@ void scoped_lock::unlock(void) { assert(locked); assert(! is_forked_child()); - VOMIT_ON_FAILURE(pthread_mutex_unlock(lock_obj)); + VOMIT_ON_FAILURE_NO_ERRNO(pthread_mutex_unlock(lock_obj)); locked = false; } @@ -2250,6 +2271,84 @@ scoped_lock::~scoped_lock() if (locked) this->unlock(); } +void scoped_rwlock::lock(void) +{ + assert(! (locked || locked_shared)); + assert(! is_forked_child()); + VOMIT_ON_FAILURE_NO_ERRNO(pthread_rwlock_rdlock(rwlock_obj)); + locked = true; +} + +void scoped_rwlock::unlock(void) +{ + assert(locked); + assert(! is_forked_child()); + VOMIT_ON_FAILURE_NO_ERRNO(pthread_rwlock_unlock(rwlock_obj)); + locked = false; +} + +void scoped_rwlock::lock_shared(void) +{ + assert(! (locked || locked_shared)); + assert(! is_forked_child()); + VOMIT_ON_FAILURE_NO_ERRNO(pthread_rwlock_wrlock(rwlock_obj)); + locked_shared = true; +} + +void scoped_rwlock::unlock_shared(void) +{ + assert(locked_shared); + assert(! is_forked_child()); + VOMIT_ON_FAILURE_NO_ERRNO(pthread_rwlock_unlock(rwlock_obj)); + locked_shared = false; +} + +void scoped_rwlock::upgrade(void) +{ + assert(locked_shared); + assert(! is_forked_child()); + VOMIT_ON_FAILURE_NO_ERRNO(pthread_rwlock_unlock(rwlock_obj)); + locked = false; + VOMIT_ON_FAILURE_NO_ERRNO(pthread_rwlock_wrlock(rwlock_obj)); + locked_shared = true; +} + +scoped_rwlock::scoped_rwlock(pthread_rwlock_t &rwlock, bool shared) : rwlock_obj(&rwlock), locked(false), locked_shared(false) +{ + if (shared) + { + this->lock_shared(); + } + else + { + this->lock(); + } +} + +scoped_rwlock::scoped_rwlock(rwlock_t &rwlock, bool shared) : rwlock_obj(&rwlock.rwlock), locked(false), locked_shared(false) +{ + if (shared) + { + this->lock_shared(); + } + else + { + this->lock(); + } +} + +scoped_rwlock::~scoped_rwlock() +{ + if (locked) + { + this->unlock(); + } + else if (locked_shared) + { + this->unlock_shared(); + } +} + wcstokenizer::wcstokenizer(const wcstring &s, const wcstring &separator) : buffer(), str(), diff --git a/common.h b/common.h index ff12760d5..84c908aaa 100644 --- a/common.h +++ b/common.h @@ -121,7 +121,9 @@ inline bool selection_direction_is_cardinal(selection_direction_t dir) /** Helper macro for errors */ -#define VOMIT_ON_FAILURE(a) do { if (0 != (a)) { int err = errno; fprintf(stderr, "%s failed on line %d in file %s: %d (%s)\n", #a, __LINE__, __FILE__, err, strerror(err)); abort(); }} while (0) +#define VOMIT_ON_FAILURE(a) do { if (0 != (a)) { VOMIT_ABORT(errno, #a); } } while (0) +#define VOMIT_ON_FAILURE_NO_ERRNO(a) do { int err = (a); if (0 != err) { VOMIT_ABORT(err, #a); } } while (0) +#define VOMIT_ABORT(err, str) do { int code = (err); fprintf(stderr, "%s failed on line %d in file %s: %d (%s)\n", str, __LINE__, __FILE__, code, strerror(code)); abort(); } while(0) /** Exits without invoking destructors (via _exit), useful for code after fork. */ void exit_without_destructors(int code) __attribute__((noreturn)); @@ -544,12 +546,12 @@ class mutex_lock_t pthread_mutex_t mutex; mutex_lock_t() { - pthread_mutex_init(&mutex, NULL); + VOMIT_ON_FAILURE_NO_ERRNO(pthread_mutex_init(&mutex, NULL)); } ~mutex_lock_t() { - pthread_mutex_destroy(&mutex); + VOMIT_ON_FAILURE_NO_ERRNO(pthread_mutex_destroy(&mutex)); } }; @@ -571,6 +573,48 @@ public: ~scoped_lock(); }; +class rwlock_t +{ +public: + pthread_rwlock_t rwlock; + rwlock_t() + { + VOMIT_ON_FAILURE_NO_ERRNO(pthread_rwlock_init(&rwlock, NULL)); + } + + ~rwlock_t() + { + VOMIT_ON_FAILURE_NO_ERRNO(pthread_rwlock_destroy(&rwlock)); + } +}; + +/* + Scoped lock class for rwlocks + */ +class scoped_rwlock +{ + pthread_rwlock_t *rwlock_obj; + bool locked; + bool locked_shared; + + /* No copying */ + scoped_rwlock &operator=(const scoped_lock &); + scoped_rwlock(const scoped_lock &); + +public: + void lock(void); + void unlock(void); + void lock_shared(void); + void unlock_shared(void); + /* + upgrade shared lock to exclusive. + equivalent to `lock.unlock_shared(); lock.lock();` + */ + void upgrade(void); + scoped_rwlock(pthread_rwlock_t &rwlock, bool shared = false); + scoped_rwlock(rwlock_t &rwlock, bool shared = false); + ~scoped_rwlock(); +}; /** A scoped manager to save the current value of some variable, and optionally diff --git a/function.cpp b/function.cpp index 5fd376974..f4ae2442e 100644 --- a/function.cpp +++ b/function.cpp @@ -67,9 +67,6 @@ void function_autoload_t::command_removed(const wcstring &cmd) function_remove_ignore_autoload(cmd); } -/* Helper macro for vomiting */ -#define VOMIT_ON_FAILURE(a) do { if (0 != (a)) { int err = errno; fprintf(stderr, "%s failed on line %d in file %s: %d (%s)\n", #a, __LINE__, __FILE__, err, strerror(err)); abort(); }} while (0) - /** Kludgy flag set by the load function in order to tell function_add that the function being defined is autoloaded. There should be a