From 1fd44b15677e062cfcc242adf25856ed8c860b8d Mon Sep 17 00:00:00 2001 From: Stefan Eissing Date: Tue, 3 Mar 2015 17:09:15 +0100 Subject: [PATCH 1/2] replacing thread_local, which does not exist on OS X, with pthread_getspecific call --- src/shrpx.cc | 8 ++++---- src/shrpx_http_downstream_connection.cc | 2 +- src/shrpx_https_upstream.cc | 2 +- src/shrpx_log.cc | 6 +++--- src/shrpx_log.h | 4 ++-- src/shrpx_log_config.cc | 23 +++++++++++++++++++++-- src/shrpx_log_config.h | 6 +----- 7 files changed, 33 insertions(+), 18 deletions(-) diff --git a/src/shrpx.cc b/src/shrpx.cc index 7121077b..141b0f55 100644 --- a/src/shrpx.cc +++ b/src/shrpx.cc @@ -1884,15 +1884,15 @@ int main(int argc, char **argv) { } if (get_config()->uid != 0) { - if (log_config->accesslog_fd != -1 && - fchown(log_config->accesslog_fd, get_config()->uid, + if (log_config()->accesslog_fd != -1 && + fchown(log_config()->accesslog_fd, get_config()->uid, get_config()->gid) == -1) { auto error = errno; LOG(WARN) << "Changing owner of access log file failed: " << strerror(error); } - if (log_config->errorlog_fd != -1 && - fchown(log_config->errorlog_fd, get_config()->uid, get_config()->gid) == + if (log_config()->errorlog_fd != -1 && + fchown(log_config()->errorlog_fd, get_config()->uid, get_config()->gid) == -1) { auto error = errno; LOG(WARN) << "Changing owner of error log file failed: " diff --git a/src/shrpx_http_downstream_connection.cc b/src/shrpx_http_downstream_connection.cc index b251f64c..f27e3029 100644 --- a/src/shrpx_http_downstream_connection.cc +++ b/src/shrpx_http_downstream_connection.cc @@ -358,7 +358,7 @@ int HttpDownstreamConnection::push_request_headers() { if (LOG_ENABLED(INFO)) { const char *hdrp; std::string nhdrs; - if (log_config->errorlog_tty) { + if (log_config()->errorlog_tty) { nhdrs = http::colorizeHeaders(hdrs.c_str()); hdrp = nhdrs.c_str(); } else { diff --git a/src/shrpx_https_upstream.cc b/src/shrpx_https_upstream.cc index 302b928f..15dd6699 100644 --- a/src/shrpx_https_upstream.cc +++ b/src/shrpx_https_upstream.cc @@ -808,7 +808,7 @@ int HttpsUpstream::on_downstream_abort_request(Downstream *downstream, void HttpsUpstream::log_response_headers(const std::string &hdrs) const { const char *hdrp; std::string nhdrs; - if (log_config->errorlog_tty) { + if (log_config()->errorlog_tty) { nhdrs = http::colorizeHeaders(hdrs.c_str()); hdrp = nhdrs.c_str(); } else { diff --git a/src/shrpx_log.cc b/src/shrpx_log.cc index 407ae5e3..955c5dc7 100644 --- a/src/shrpx_log.cc +++ b/src/shrpx_log.cc @@ -99,7 +99,7 @@ Log::~Log() { return; } - auto lgconf = log_config; + auto lgconf = log_config(); if (!log_enabled(severity_) || (lgconf->errorlog_fd == -1 && !get_config()->errorlog_syslog)) { @@ -159,7 +159,7 @@ std::pair copy(const char *src, size_t avail, } // namespace void upstream_accesslog(const std::vector &lfv, LogSpec *lgsp) { - auto lgconf = log_config; + auto lgconf = log_config(); if (lgconf->accesslog_fd == -1 && !get_config()->accesslog_syslog) { return; @@ -272,7 +272,7 @@ void upstream_accesslog(const std::vector &lfv, LogSpec *lgsp) { int reopen_log_files() { int res = 0; - auto lgconf = log_config; + auto lgconf = log_config(); if (lgconf->accesslog_fd != -1) { close(lgconf->accesslog_fd); diff --git a/src/shrpx_log.h b/src/shrpx_log.h index 01358297..14604633 100644 --- a/src/shrpx_log.h +++ b/src/shrpx_log.h @@ -97,8 +97,8 @@ private: static int severity_thres_; }; -#define TTY_HTTP_HD (log_config->errorlog_tty ? "\033[1;34m" : "") -#define TTY_RST (log_config->errorlog_tty ? "\033[0m" : "") +#define TTY_HTTP_HD (log_config()->errorlog_tty ? "\033[1;34m" : "") +#define TTY_RST (log_config()->errorlog_tty ? "\033[0m" : "") enum LogFragmentType { SHRPX_LOGF_NONE, diff --git a/src/shrpx_log_config.cc b/src/shrpx_log_config.cc index 24fa15b5..0bd5a1f7 100644 --- a/src/shrpx_log_config.cc +++ b/src/shrpx_log_config.cc @@ -33,9 +33,28 @@ LogConfig::LogConfig() : accesslog_fd(-1), errorlog_fd(-1), errorlog_tty(false) {} #ifndef NOTHREADS -thread_local + static pthread_key_t lckey; + static pthread_once_t lckey_once = PTHREAD_ONCE_INIT; + + static void make_key(void) { + pthread_key_create(&lckey, NULL); + } + + LogConfig *log_config(void) { + pthread_once(&lckey_once, make_key); + LogConfig *config = (LogConfig *)pthread_getspecific(lckey); + if (!config) { + config = new LogConfig(); + pthread_setspecific(lckey, config); + } + return config; + } +#else + static LogConfig *config = new LogConfig(); + LogConfig *log_config(void) { + return config; + } #endif // NOTHREADS - LogConfig *log_config = new LogConfig(); void LogConfig::update_tstamp(const std::chrono::system_clock::time_point &now) { diff --git a/src/shrpx_log_config.h b/src/shrpx_log_config.h index 3fcc9b9e..46b6357e 100644 --- a/src/shrpx_log_config.h +++ b/src/shrpx_log_config.h @@ -46,11 +46,7 @@ struct LogConfig { // We need LogConfig per thread to avoid data race around opening file // descriptor for log files. -extern -#ifndef NOTHREADS - thread_local -#endif // NOTHREADS - LogConfig *log_config; +extern LogConfig *log_config(void); } // namespace shrpx From 7f802b623de0020cfaa6028edc02c9efa1fc705b Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Thu, 5 Mar 2015 02:06:31 +0900 Subject: [PATCH 2/2] Remove thread_local check, since we use pthread_* directly for now --- configure.ac | 22 +++------------------- 1 file changed, 3 insertions(+), 19 deletions(-) diff --git a/configure.ac b/configure.ac index 758e4b71..5311cdd4 100644 --- a/configure.ac +++ b/configure.ac @@ -200,21 +200,6 @@ std::vector> v; [have_std_future=no AC_MSG_RESULT([no])]) -# Check that thread_local is available. -AC_MSG_CHECKING([whether thread_local is available]) -AC_COMPILE_IFELSE([AC_LANG_PROGRAM( -[[ -]], -[[ -thread_local int n; -]])], - [AC_DEFINE([HAVE_THREAD_LOCAL], [1], - [Define to 1 if you have the `thread_local`.]) - have_thread_local=yes - AC_MSG_RESULT([yes])], - [have_thread_local=no - AC_MSG_RESULT([no])]) - # Check that std::map::emplace is available for g++-4.7. AC_MSG_CHECKING([whether std::map::emplace is available]) AC_COMPILE_IFELSE([AC_LANG_PROGRAM( @@ -631,11 +616,10 @@ if test "x$debug" != "xno"; then fi enable_threads=yes -# Some platform does not have working std::future or thread_local. We -# disable threading for those platforms. +# Some platform does not have working std::future. We disable +# threading for those platforms. if test "x$threads" != "xyes" || - test "x$have_std_future" != "xyes" || - test "x$have_thread_local" != "xyes"; then + test "x$have_std_future" != "xyes"; then enable_threads=no AC_DEFINE([NOTHREADS], [1], [Define to 1 if you want to disable threads.]) fi