Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LOGCXX-431 #68

Merged
merged 17 commits into from
Aug 30, 2021
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/cmake/pthread/log4cxx-pthread.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
include(FindThreads)

try_compile(PTHREAD_SETNAME_NP_FOUND "${CMAKE_BINARY_DIR}/pthread-compile-tests"
"${CMAKE_CURRENT_LIST_DIR}/test-pthread.cpp"
LINK_LIBRARIES Threads::Threads )

6 changes: 6 additions & 0 deletions src/cmake/pthread/test-pthread.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
#include <pthread.h>

int main(){
pthread_t tid;
pthread_setname_np(tid, "name");
}
1 change: 1 addition & 0 deletions src/main/cpp/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@ target_sources(log4cxx
threadlocal.cpp
threadpatternconverter.cpp
threadspecificdata.cpp
threadutility.cpp
throwableinformationpatternconverter.cpp
timebasedrollingpolicy.cpp
timezone.cpp
Expand Down
3 changes: 2 additions & 1 deletion src/main/cpp/asyncappender.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#include <log4cxx/helpers/stringhelper.h>
#include <apr_atomic.h>
#include <log4cxx/helpers/optionconverter.h>
#include <log4cxx/helpers/threadutility.h>


using namespace log4cxx;
Expand All @@ -50,7 +51,7 @@ AsyncAppender::AsyncAppender()
locationInfo(false),
blocking(true)
{
dispatcher = std::thread( &AsyncAppender::dispatch, this );
dispatcher = ThreadUtility::instance()->createThread( LOG4CXX_STR("AyncAppend"), &AsyncAppender::dispatch, this );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AyncAppend vs. AsyncAppend vs. AsyncAppender vs. SocketAppend ... There's at least an s missing.

In general: Did you choose to not use the class name or should that simply be reused 1:1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Linux at least, the thread name is limited to 16 characters, so I wanted to keep the names as short as possible. At least in this case, there's enough space to do 'AsyncAppender'.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't aware of the length limit, so your shorter names make perfectly sense. You are using them in most cases anyway already, no need to make it different here:

  • AsyncAppend vs. AsyncAppender
  • FileWatch vs. FileWatchdog
  • SocketAppend vs. SocketAppenderSkeleton
  • SocketHub vs. SocketHubAppender
  • TelnetAppend vs. TelnetAppender

}

AsyncAppender::~AsyncAppender()
Expand Down
3 changes: 2 additions & 1 deletion src/main/cpp/filewatchdog.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include <apr_atomic.h>
#include <log4cxx/helpers/transcoder.h>
#include <log4cxx/helpers/exception.h>
#include <log4cxx/helpers/threadutility.h>
#include <functional>

using namespace log4cxx;
Expand Down Expand Up @@ -92,7 +93,7 @@ void FileWatchdog::start()
{
checkAndConfigure();

thread = std::thread( &FileWatchdog::run, this );
thread = ThreadUtility::instance()->createThread( LOG4CXX_STR("Filewatchdog"), &FileWatchdog::run, this );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filewatchdog vs. FileWatchdog vs. AsyncAppender vs. SocketAppend ... Lower case looks like a mistake.

}

bool FileWatchdog::is_interrupted()
Expand Down
3 changes: 2 additions & 1 deletion src/main/cpp/socketappenderskeleton.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#include <log4cxx/spi/loggingevent.h>
#include <log4cxx/helpers/transcoder.h>
#include <log4cxx/helpers/bytearrayoutputstream.h>
#include <log4cxx/helpers/threadutility.h>
#include <functional>

using namespace log4cxx;
Expand Down Expand Up @@ -162,7 +163,7 @@ void SocketAppenderSkeleton::fireConnector()
{
LogLog::debug(LOG4CXX_STR("Connector thread not alive: starting monitor."));

thread = std::thread( &SocketAppenderSkeleton::monitor, this );
thread = ThreadUtility::instance()->createThread( LOG4CXX_STR("SocketAppend"), &SocketAppenderSkeleton::monitor, this );
}
}

Expand Down
3 changes: 2 additions & 1 deletion src/main/cpp/sockethubappender.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#include <log4cxx/helpers/objectoutputstream.h>
#include <log4cxx/helpers/socketoutputstream.h>
#include <log4cxx/helpers/exception.h>
#include <log4cxx/helpers/threadutility.h>
#include <mutex>

using namespace log4cxx;
Expand Down Expand Up @@ -177,7 +178,7 @@ void SocketHubAppender::append(const spi::LoggingEventPtr& event, Pool& p)

void SocketHubAppender::startServer()
{
thread = std::thread( &SocketHubAppender::monitor, this );
thread = ThreadUtility::instance()->createThread( LOG4CXX_STR("SocketHub"), &SocketHubAppender::monitor, this );
}

void SocketHubAppender::monitor()
Expand Down
3 changes: 2 additions & 1 deletion src/main/cpp/telnetappender.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include <apr_strings.h>
#include <log4cxx/helpers/charsetencoder.h>
#include <log4cxx/helpers/bytebuffer.h>
#include <log4cxx/helpers/threadutility.h>
#include <mutex>

using namespace log4cxx;
Expand Down Expand Up @@ -61,7 +62,7 @@ void TelnetAppender::activateOptions(Pool& /* p */)
serverSocket->setSoTimeout(1000);
}

sh = std::thread( &TelnetAppender::acceptConnections, this );
sh = ThreadUtility::instance()->createThread( LOG4CXX_STR("TelnetAppend"), &TelnetAppender::acceptConnections, this );
}

void TelnetAppender::setOption(const LogString& option,
Expand Down
91 changes: 91 additions & 0 deletions src/main/cpp/threadutility.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
#include "log4cxx/helpers/threadutility.h"
#include "log4cxx/private/log4cxx_private.h"
#include "log4cxx/helpers/loglog.h"

#include <signal.h>

#if LOG4CXX_HAS_SETTHREADDESCRIPTION
#include <windows.h>
#include <processthreadsapi.h>
#endif

using log4cxx::helpers::ThreadUtility;

struct ThreadUtility::priv_data{
ams-tschoening marked this conversation as resolved.
Show resolved Hide resolved
priv_data(){
pre_start = nullptr;
thread_start = nullptr;
post_start = nullptr;
}

log4cxx::helpers::pre_thread_start pre_start;
log4cxx::helpers::thread_started thread_start;
log4cxx::helpers::post_thread_start post_start;
};
ams-tschoening marked this conversation as resolved.
Show resolved Hide resolved

ThreadUtility::ThreadUtility() :
m_priv( new priv_data() )
{}

ThreadUtility::~ThreadUtility(){}

std::shared_ptr<ThreadUtility> ThreadUtility::instance(){
static std::shared_ptr<ThreadUtility> instance( new ThreadUtility() );
return instance;
}

void ThreadUtility::configureThreadFunctions( pre_thread_start pre_start1,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opinion: The phrase thread in your function names reads redundant and unnecessary noisy to me, as that phrase is already part of the class name.

Opinion: function[s] used at multiple places reads unnecessary long to me. A simple func[s] would be sufficient in my opinion.

thread_started started,
post_thread_start post_start1 ){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for 1? Makes me expect at least a 2. :-) If to avoid name clashes, how about prefixing with _ instead? I have the feeling that's a widely adopted convention, though, more likely used for the instance variables instead of the arguments. But is a different name necessary at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that I had named things the same before; I can probably fix that easily enough.

m_priv->pre_start = pre_start1;
m_priv->thread_start = started;
m_priv->post_start = post_start1;
}

void ThreadUtility::preThreadBlockSignals(){
#if LOG4CXX_HAS_PTHREAD_SIGMASK
sigset_t set;
sigfillset(&set);
if( pthread_sigmask(SIG_SETMASK, &set, nullptr) < 0 ){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you want SIG_BLOCK (to be more explicit) here and store the old set somewhere....

LOGLOG_ERROR( LOG4CXX_STR("Unable to set thread sigmask") );
}
#endif /* LOG4CXX_HAS_PTHREAD_SIGMASK */
}

void ThreadUtility::threadStartedNameThread(LogString threadName,
std::thread::id /*thread_id*/,
std::thread::native_handle_type native_handle){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opinion: threadName vs. thread_name vs. native_handle vs. nativeHandle

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at it again, I also realized that I had done cpp_style instead of JavaStyle for the function definitions as well. Since the rest of the library is all JavaStyle, I've made those updates as well.

#if LOG4CXX_HAS_PTHREAD_SETNAME
if( pthread_setname_np( static_cast<pthread_t>( native_handle ), threadName.c_str() ) < 0 ){
LOGLOG_ERROR( LOG4CXX_STR("unable to set thread name") );
}
#elif LOG4CXX_HAS_SETTHREADDESCRIPTION
HRESULT hr = SetThreadDescription(static_cast<HANDLE>(native_handle), threadName.c_str());
if(FAILED(hr)){
LOGLOG_ERROR( LOG4CXX_STR("unable to set thread name") );
}
#endif
}

void ThreadUtility::postThreadUnblockSignals(){
#if LOG4CXX_HAS_PTHREAD_SIGMASK
sigset_t set;
sigemptyset(&set);
if( pthread_sigmask(SIG_SETMASK, &set, nullptr) < 0 ){
LOGLOG_ERROR( LOG4CXX_STR("Unable to set thread sigmask") );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... so that you can restore the original sigmask afterwards....

}
#endif /* LOG4CXX_HAS_PTHREAD_SIGMASK */
}


log4cxx::helpers::pre_thread_start ThreadUtility::preStartFunction(){
return m_priv->pre_start;
}

log4cxx::helpers::thread_started ThreadUtility::threadStartedFunction(){
return m_priv->thread_start;
}

log4cxx::helpers::post_thread_start ThreadUtility::postStartFunction(){
return m_priv->post_start;
}
20 changes: 19 additions & 1 deletion src/main/include/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,26 @@ CHECK_FUNCTION_EXISTS(wcstombs HAS_WCSTOMBS)
CHECK_FUNCTION_EXISTS(fwide HAS_FWIDE)
CHECK_LIBRARY_EXISTS(esmtp smtp_create_session "" HAS_LIBESMTP)
CHECK_FUNCTION_EXISTS(syslog HAS_SYSLOG)
if(UNIX)
set(CMAKE_REQUIRED_LIBRARIES "pthread")
CHECK_SYMBOL_EXISTS(pthread_sigmask "signal.h" HAS_PTHREAD_SIGMASK)

# Check for the (linux) pthread_setname_np.
# OSX and BSD are special apparently. OSX only lets you name
# the current thread, while BSD calls it pthread_set_name_np.
# Since this is really not a core feature and the end-user can configure
# it anyway, we're only going to worry about linux.
include(${LOG4CXX_SOURCE_DIR}/src/cmake/pthread/log4cxx-pthread.cmake)
if(${PTHREAD_SETNAME_NP_FOUND})
set(HAS_PTHREAD_SETNAME 1)
endif()
endif(UNIX)

foreach(varName HAS_STD_LOCALE HAS_ODBC HAS_MBSRTOWCS HAS_WCSTOMBS HAS_FWIDE HAS_LIBESMTP HAS_SYSLOG)
if(WIN32)
CHECK_SYMBOL_EXISTS(SetThreadDescription "windows.h;processthreadsapi.h" HAS_SETTHREADDESCRIPTION)
endif(WIN32)

foreach(varName HAS_STD_LOCALE HAS_ODBC HAS_MBSRTOWCS HAS_WCSTOMBS HAS_FWIDE HAS_LIBESMTP HAS_SYSLOG HAS_PTHREAD_SIGMASK HAS_PTHREAD_SETNAME HAS_SETTHREADDESCRIPTION)
if(${varName} EQUAL 0)
continue()
elseif(${varName} EQUAL 1)
Expand Down
119 changes: 119 additions & 0 deletions src/main/include/log4cxx/helpers/threadutility.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
#ifndef _LOG4CXX_THREADUTILITY_H
#define _LOG4CXX_THREADUTILITY_H

#include <thread>
#include <functional>
#include <memory>

#include "log4cxx/logstring.h"

namespace log4cxx {
namespace helpers {

/**
* A function that will be called before a thread is started. This can
* be used to (for example) block all of the signals in the thread, so
* that when the thread is created it will have a correct signal mask.
*/
typedef std::function<void()> pre_thread_start;

/**
* Called when a new thread has started. This can be used to set
* parameters for the thread in a platform-specific manner.
*
* @param threadName The name of the thread
* @param thread_id The ID of the thread as reported by std::thread::get_id
* @param native_handle The native handle of the thread, as reported by
* std::thread::native_handle
*/
typedef std::function<void( LogString threadName,
std::thread::id thread_id,
std::thread::native_handle_type native_handle )> thread_started;

/**
* Called after a thread has started. This can be used to (for example)
* unblock the signals in the thread.
*/
typedef std::function<void()> post_thread_start;

class ThreadUtility;
LOG4CXX_PTR_DEF(ThreadUtility);

class LOG4CXX_EXPORT ThreadUtility {
private:
ThreadUtility();

log4cxx::helpers::pre_thread_start preStartFunction();
log4cxx::helpers::thread_started threadStartedFunction();
log4cxx::helpers::post_thread_start postStartFunction();

struct priv_data;
std::unique_ptr<priv_data> m_priv;

public:
~ThreadUtility();

static std::shared_ptr<ThreadUtility> instance();

/**
* Configure the thread functions that log4cxx will use.
* Note that setting any of these parameters to nullptr is valid,
* and simply results in the callback not being called. *
*/
void configureThreadFunctions( pre_thread_start pre_start,
thread_started started,
post_thread_start post_start );

/**
* A pre-start thread function that blocks signals to the new thread
* (if the system has pthreads). If the system does not have pthreads,
* is equivalent to preThreadDoNothing();
*/
void preThreadBlockSignals();

/**
* A thread_started function that names the thread using the appropriate
* system call.
*/
void threadStartedNameThread(LogString threadName,
std::thread::id thread_id,
std::thread::native_handle_type native_handle);

/**
* A post-start thread function that unblocks signals that preThreadBlockSignals
* blocked before starting the thread. If the system does not have pthreads,
* is equivalent to postThreadDoNothing();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't find an implementation of postThreadDoNothing. Did I miss something or is the function missing or the comment outdated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Outdated comment; I originally had a method that did nothing, but then I realized that just testing the std::function to see if it is valid does the same thing.

*/
void postThreadUnblockSignals();

/**
* Start a thread
*/
template<class Function, class... Args>
std::thread createThread(LogString name,
Function&& f,
Args&&... args){
log4cxx::helpers::pre_thread_start pre_start = preStartFunction();
log4cxx::helpers::thread_started thread_start = threadStartedFunction();
log4cxx::helpers::post_thread_start post_start = postStartFunction();

if( pre_start ){
pre_start();
}
std::thread t( f, args... );
if( thread_start ){
thread_start( name,
t.get_id(),
t.native_handle() );
}
if( post_start ){
ams-tschoening marked this conversation as resolved.
Show resolved Hide resolved
post_start();
}
return t;
}
};

} /* namespace helpers */
} /* namespace log4cxx */

#endif
4 changes: 4 additions & 0 deletions src/main/include/log4cxx/private/log4cxx_private.h.in
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,10 @@
#define LOG4CXX_WIN32_THREAD_FMTSPEC "0x%.8x"
#define LOG4CXX_APR_THREAD_FMTSPEC "0x%pt"

#define LOG4CXX_HAS_PTHREAD_SIGMASK @HAS_PTHREAD_SIGMASK@
#define LOG4CXX_HAS_PTHREAD_SETNAME @HAS_PTHREAD_SETNAME@
#define LOG4CXX_HAS_SETTHREADDESCRIPTION @HAS_SETTHREADDESCRIPTION@

#ifdef __BORLANDC__
/*
* embarcadero/RAD Studio compilers don't support thread_local:
Expand Down
1 change: 1 addition & 0 deletions src/site/markdown/1-usage.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ Usage {#usage-overview}
See the following pages for usage information:

* @subpage usage
* @subpage threading
* @subpage extending-log4cxx
* @subpage faq
* @subpage configuration-samples
Expand Down