kicad-developers team mailing list archive
-
kicad-developers team
-
Mailing list archive
-
Message #22580
Fwd: [PATCH] github_plugin -> curl -> openssl thread safety
Would someone please test this patch on osx and on a linux distro that
uses a libcurl variant built against ntls or gnutls. I will test it on
windows. Please read the comments below carefully and do not reply to
Dick directly.
-------- Forwarded Message --------
Subject: [PATCH] github_plugin -> curl -> openssl thread safety
Date: Wed, 13 Jan 2016 00:07:35 -0600
From: Dick Hollenbeck <dick@xxxxxxxxxxx>
To: Wayne Stambaugh <stambaughw@xxxxxxxxxxx>, jp.charras@xxxxxxxxxx
<jp.charras@xxxxxxxxxx>
Guys, this brings in the necessary locks required to make openssl thread
safe. I think it completes the migration to libcurl. As I reported to
Wayne, even on linux I was seeing evidence of unprotected critical
sections of code. Going down to 6 threads merely swept the problem
under a rug but did not fix the fundamental problem. This does.
Six threads still seems about the optimal mix of chaos and throughput.
You may want to bring back the FindOpenSSL.cmake script which this must
use, but I got by with CMake's default. Please just do that as a
separate patch since I am not responsible for it. You could also do any
Windows specific fixups as a separate patch also, as it does not
attribute stuff to me that I did not do, and vice versa.
HTH in fulfilling your vision for the github plugin. The locking does
not affect the speed by much, and multiple threads are still a
tremendous speed boost. I still would never use the plugin without
nginx, and I think that should be made more clear to those who wine
about speed. There is an awful lot of whining, and none of these people
even seem to know about nginx. nginx + github plugin is even faster
than the PCB_IO plugin.
I think I am loading about 80 libraries in 2 seconds. Only two of those
are PCB_IO, the rest are github.
PLEASE MAKE THIS KNOWN!
*) Switch to static linking of libcurl and on linux and windows and also
statically link in only required portions of openssl.
*) Add the required thread locks which openssl needs.
*) Remove the get curl version call from BASEFRAME since it pulls in
curl and openssl
into every derived wxFrame class link image.
*) Remove curl function from PGM_BASE, switch to atexit() instead.
Anything in PGM_BASE made the singletops bigger.
*) Tested on Linux only.
From eaa66357243579daeef96ae0f523ebc76f7775f2 Mon Sep 17 00:00:00 2001
From: Dick Hollenbeck <dick@xxxxxxxxxxx>
Date: Tue, 12 Jan 2016 22:56:48 -0600
Subject: *) Switch to static linking of libcurl and on linux and windows also
openssl. *) Add the required thread locks which openssl needs. *) Remove the
get curl version call from BASEFRAME since it pulls in curl and openssl
into every derived wxFrame class link image.
diff --git a/common/CMakeLists.txt b/common/CMakeLists.txt
index cf66392..12f823e 100644
--- a/common/CMakeLists.txt
+++ b/common/CMakeLists.txt
@@ -12,6 +12,12 @@ include_directories(
${INC_AFTER}
)
+
+if( NOT APPLE ) # windows and linux use openssl under curl
+ find_package( OpenSSL REQUIRED )
+endif()
+
+
# Generate header files containing shader programs
# Order of input files is significant
add_custom_command(
@@ -285,7 +291,8 @@ add_dependencies( common lib-dependencies )
add_dependencies( common version_header )
target_link_libraries( common
${Boost_LIBRARIES}
-# ${CURL_LIBRARIES} we dynamically link to this ON DEMAND, not at load time
+ ${CURL_LIBRARIES}
+ ${OPENSSL_LIBRARIES} # empty on Apple
)
diff --git a/common/basicframe.cpp b/common/basicframe.cpp
index e39a5b2..05af429 100644
--- a/common/basicframe.cpp
+++ b/common/basicframe.cpp
@@ -28,7 +28,6 @@
* @brief EDA_BASE_FRAME class implementation.
*/
-#include <kicad_curl/kicad_curl.h> /* Include before any wx file */
#include <wx/aboutdlg.h>
#include <wx/fontdlg.h>
#include <wx/clipbrd.h>
@@ -574,8 +573,6 @@ void EDA_BASE_FRAME::CopyVersionInfoToClipboard( wxCommandEvent& event )
<< ( BOOST_VERSION / 100 % 1000 ) << wxT( "." )
<< ( BOOST_VERSION % 100 ) << wxT( "\n" );
- msg_version << KICAD_CURL::GetSimpleVersion() << wxT( "\n" );
-
msg_version << wxT( " USE_WX_GRAPHICS_CONTEXT=" );
#ifdef USE_WX_GRAPHICS_CONTEXT
msg_version << wxT( "ON\n" );
diff --git a/common/kicad_curl/kicad_curl.cpp b/common/kicad_curl/kicad_curl.cpp
index 0977f28..3201bf7 100644
--- a/common/kicad_curl/kicad_curl.cpp
+++ b/common/kicad_curl/kicad_curl.cpp
@@ -2,6 +2,7 @@
* This program source code file is part of KiCad, a free EDA CAD application.
*
* Copyright (C) 2015 Mark Roszko <mark.roszko@xxxxxxxxx>
+ * Copyright (C) 2016 SoftPLC Corporation, Dick Hollenbeck <dick@xxxxxxxxxxx>
* Copyright (C) 2015 KiCad Developers, see CHANGELOG.TXT for contributors.
*
* This program is free software; you can redistribute it and/or
@@ -26,56 +27,106 @@
#include <wx/dynlib.h>
#include <macros.h>
+#include <fctsys.h>
#include <kicad_curl/kicad_curl.h>
#include <ki_mutex.h> // MUTEX and MUTLOCK
#include <richio.h>
+
+
// These are even more private than class members, and since there is only
// one instance of KICAD_CURL ever, these statics are hidden here to simplify the
// client (API) header file.
static volatile bool s_initialized;
-static MUTEX s_lock;
+static MUTEX s_lock; // for s_initialized
+
+// Assume that on these platforms libcurl uses OpenSSL
+#if defined(__linux__) || defined(_WIN32)
+
+#include <openssl/crypto.h>
+
+static MUTEX* s_crypto_locks;
+
+static void lock_callback( int mode, int type, const char* file, int line )
+{
+ (void)file;
+ (void)line;
+
+ wxASSERT( s_crypto_locks && unsigned( type ) < unsigned( CRYPTO_num_locks() ) );
+
+ //DBG( printf( "%s: mode=0x%x type=%d file=%s line=%d\n", __func__, mode, type, file, line );)
+
+ if( mode & CRYPTO_LOCK )
+ {
+ s_crypto_locks[ type ].lock();
+ }
+ else
+ {
+ s_crypto_locks[ type ].unlock();
+ }
+}
+
+static void init_locks()
+{
+ s_crypto_locks = new MUTEX[ CRYPTO_num_locks() ];
+
+ // From http://linux.die.net/man/3/crypto_set_id_callback:
-void (CURL_EXTERN * KICAD_CURL::easy_cleanup) ( CURL* curl );
-CURL* (CURL_EXTERN * KICAD_CURL::easy_init) ( void );
-CURLcode (CURL_EXTERN * KICAD_CURL::easy_perform) ( CURL* curl );
-CURLcode (CURL_EXTERN * KICAD_CURL::easy_setopt) ( CURL* curl, CURLoption option, ... );
-const char* (CURL_EXTERN * KICAD_CURL::easy_strerror) ( CURLcode );
-CURLcode (CURL_EXTERN * KICAD_CURL::global_init) ( long flags );
-void (CURL_EXTERN * KICAD_CURL::global_cleanup) ( void );
-curl_slist* (CURL_EXTERN * KICAD_CURL::slist_append) ( curl_slist*, const char* );
-void (CURL_EXTERN * KICAD_CURL::slist_free_all) ( curl_slist* );
-char* (CURL_EXTERN * KICAD_CURL::version) ( void );
-curl_version_info_data* (CURL_EXTERN * KICAD_CURL::version_info) (CURLversion);
+ /*
+ OpenSSL can safely be used in multi-threaded applications provided that at
+ least two callback functions are set, locking_function and threadid_func.
+
+ locking_function(int mode, int n, const char *file, int line) is needed to
+ perform locking on shared data structures. (Note that OpenSSL uses a number
+ of global data structures that will be implicitly shared whenever multiple
+ threads use OpenSSL.) Multi-threaded applications will crash at random if it
+ is not set.
+
+ threadid_func( CRYPTO_THREADID *id) is needed to record the
+ currently-executing thread's identifier into id. The implementation of this
+ callback should not fill in id directly, but should use
+ CRYPTO_THREADID_set_numeric() if thread IDs are numeric, or
+ CRYPTO_THREADID_set_pointer() if they are pointer-based. If the application
+ does not register such a callback using CRYPTO_THREADID_set_callback(), then
+ a default implementation is used - on Windows and BeOS this uses the
+ system's default thread identifying APIs, and on all other platforms it uses
+ the address of errno. The latter is satisfactory for thread-safety if and
+ only if the platform has a thread-local error number facility.
+
+ Dick: "sounds like CRYPTO_THREADID_set_callback() is not mandatory on our
+ 2 OpenSSL platforms."
-struct DYN_LOOKUP
+ */
+
+ CRYPTO_set_locking_callback( &lock_callback );
+}
+
+
+static void kill_locks()
{
- const char* name;
- void** address;
-};
-
-// May need to modify "name" for each platform according to how libcurl is built on
-// that platform and the spelling or partial mangling of C function names. On linux
-// there is no mangling.
-#define DYN_PAIR( basename ) { "curl_" #basename, (void**) &KICAD_CURL::basename }
-
-
-const DYN_LOOKUP KICAD_CURL::dyn_funcs[] = {
- DYN_PAIR( easy_cleanup ),
- DYN_PAIR( easy_init ),
- DYN_PAIR( easy_perform ),
- DYN_PAIR( easy_setopt ),
- DYN_PAIR( easy_strerror ),
- DYN_PAIR( global_init ),
- DYN_PAIR( global_cleanup ),
- DYN_PAIR( slist_append ),
- DYN_PAIR( slist_free_all ),
- DYN_PAIR( version ),
- DYN_PAIR( version_info ),
-};
+ CRYPTO_set_locking_callback( NULL );
+
+ delete[] s_crypto_locks;
+
+ s_crypto_locks = NULL;
+}
+
+#else
+
+inline void init_locks() { /* dummy */ }
+inline void kill_locks() { /* dummy */ }
+
+#endif
+
+/// At process termination, using atexit() keeps the CURL stuff out of the
+/// singletops and PGM_BASE.
+static void at_terminate()
+{
+ KICAD_CURL::Cleanup();
+}
void KICAD_CURL::Init()
@@ -89,56 +140,14 @@ void KICAD_CURL::Init()
if( !s_initialized )
{
- // dynamically load the library.
- wxDynamicLibrary dso;
- wxString canonicalName = dso.CanonicalizeName( wxT( "curl" ) );
-
- // This is an ugly hack for MinGW builds. We should probably use something
- // like objdump to get the actual library file name from the link file.
-#if defined( __MINGW32__ )
- canonicalName = dso.CanonicalizeName( wxT( "curl-4" ) );
- canonicalName = wxT( "lib" ) + canonicalName;
-#endif
-
- if( !dso.Load( canonicalName, wxDL_NOW | wxDL_GLOBAL ) )
- {
- // Failure: error reporting UI was done via wxLogSysError().
- std::string msg = StrPrintf( "%s not wxDynamicLibrary::Load()ed",
- static_cast< const char *>( canonicalName.mb_str() ) );
- THROW_IO_ERROR( msg );
- }
-
- // get addresses.
-
- for( unsigned i=0; i < DIM(dyn_funcs); ++i )
- {
- *dyn_funcs[i].address = dso.GetSymbol( dyn_funcs[i].name );
-
- if( *dyn_funcs[i].address == NULL )
- {
- // Failure: error reporting UI was done via wxLogSysError().
- // No further reporting required here.
-
- std::string msg = StrPrintf( "%s has no function %s",
- static_cast<const char*>( canonicalName.mb_str() ),
- dyn_funcs[i].name );
-
- THROW_IO_ERROR( msg );
- }
- }
-
- if( KICAD_CURL::global_init( CURL_GLOBAL_ALL ) != CURLE_OK )
+ if( curl_global_init( CURL_GLOBAL_ALL ) != CURLE_OK )
{
THROW_IO_ERROR( "curl_global_init() failed." );
}
- wxLogDebug( "Using %s", GetVersion() );
+ init_locks();
- // Tell dso's wxDynamicLibrary destructor not to Unload() the program image,
- // since everything is fine before this. In those cases where THROW_IO_ERROR
- // is called, dso is destroyed and the DSO/DLL is unloaded before returning in
- // those error cases.
- (void) dso.Detach();
+ wxLogDebug( "Using %s", GetVersion() );
s_initialized = true;
}
@@ -169,14 +178,11 @@ void KICAD_CURL::Cleanup()
if( s_initialized )
{
+ curl_global_cleanup();
- KICAD_CURL::global_cleanup();
+ kill_locks();
- // dyn_funcs are not good for anything now, assuming process is ending soon here.
- for( unsigned i=0; i < DIM(dyn_funcs); ++i )
- {
- *dyn_funcs[i].address = 0;
- }
+ atexit( &at_terminate );
s_initialized = false;
}
@@ -189,7 +195,7 @@ std::string KICAD_CURL::GetSimpleVersion()
if( !s_initialized )
Init();
- curl_version_info_data *info = KICAD_CURL::version_info( CURLVERSION_NOW );
+ curl_version_info_data* info = curl_version_info( CURLVERSION_NOW );
std::string res;
diff --git a/common/kicad_curl/kicad_curl_easy.cpp b/common/kicad_curl/kicad_curl_easy.cpp
index e180ca6..1e84afe 100644
--- a/common/kicad_curl/kicad_curl_easy.cpp
+++ b/common/kicad_curl/kicad_curl_easy.cpp
@@ -52,29 +52,24 @@ KICAD_CURL_EASY::KICAD_CURL_EASY() :
KICAD_CURL::Init();
- // Do not catch exception from KICAD_CURL::Init() at this level.
- // Instantiation of this instance will fail if Init() throws, thus ensuring
- // that this instance cannot be subsequently used.
- // Caller needs a try catch around KICAD_CURL_EASY instantiation.
-
- m_CURL = KICAD_CURL::easy_init();
+ m_CURL = curl_easy_init();
if( !m_CURL )
{
THROW_IO_ERROR( "Unable to initialize CURL session" );
}
- KICAD_CURL::easy_setopt( m_CURL, CURLOPT_WRITEFUNCTION, write_callback );
- KICAD_CURL::easy_setopt( m_CURL, CURLOPT_WRITEDATA, (void*) &m_buffer );
+ curl_easy_setopt( m_CURL, CURLOPT_WRITEFUNCTION, write_callback );
+ curl_easy_setopt( m_CURL, CURLOPT_WRITEDATA, (void*) &m_buffer );
}
KICAD_CURL_EASY::~KICAD_CURL_EASY()
{
if( m_headers )
- KICAD_CURL::slist_free_all( m_headers );
+ curl_slist_free_all( m_headers );
- KICAD_CURL::easy_cleanup( m_CURL );
+ curl_easy_cleanup( m_CURL );
}
@@ -82,13 +77,13 @@ void KICAD_CURL_EASY::Perform()
{
if( m_headers )
{
- KICAD_CURL::easy_setopt( m_CURL, CURLOPT_HTTPHEADER, m_headers );
+ curl_easy_setopt( m_CURL, CURLOPT_HTTPHEADER, m_headers );
}
// bonus: retain worst case memory allocation, should re-use occur
m_buffer.clear();
- CURLcode res = KICAD_CURL::easy_perform( m_CURL );
+ CURLcode res = curl_easy_perform( m_CURL );
if( res != CURLE_OK )
{
diff --git a/common/pgm_base.cpp b/common/pgm_base.cpp
index ddfdf00..039a857 100644
--- a/common/pgm_base.cpp
+++ b/common/pgm_base.cpp
@@ -30,7 +30,6 @@
* (locale handling)
*/
-#include <kicad_curl/kicad_curl.h> /* Include before any wx file */
#include <fctsys.h>
#include <wx/html/htmlwin.h>
#include <wx/fs_zip.h>
@@ -290,8 +289,6 @@ void PGM_BASE::destroy()
{
// unlike a normal destructor, this is designed to be called more than once safely:
- KICAD_CURL::Cleanup();
-
delete m_common_settings;
m_common_settings = 0;
diff --git a/include/kicad_curl/kicad_curl.h b/include/kicad_curl/kicad_curl.h
index 1b8af48..51655a9 100644
--- a/include/kicad_curl/kicad_curl.h
+++ b/include/kicad_curl/kicad_curl.h
@@ -92,7 +92,7 @@ public:
*/
static const char* GetVersion()
{
- return KICAD_CURL::version();
+ return curl_version();
}
@@ -103,26 +103,6 @@ public:
* @return std::string - Generated version string
*/
static std::string GetSimpleVersion();
-private:
-
- // Alphabetically:
- // dynamically looked up libcurl function pointers whose prototypes were
- // taken from the system's libcurl headers.
-
- static void (CURL_EXTERN * easy_cleanup) ( CURL* curl );
- static CURL* (CURL_EXTERN * easy_init) ( void );
- static CURLcode (CURL_EXTERN * easy_perform) ( CURL* curl );
- static CURLcode (CURL_EXTERN * easy_setopt) ( CURL* curl, CURLoption option, ... );
- static const char* (CURL_EXTERN * easy_strerror) ( CURLcode );
- static CURLcode (CURL_EXTERN * global_init) ( long flags );
- static void (CURL_EXTERN * global_cleanup) ( void );
- static curl_slist* (CURL_EXTERN * slist_append) ( curl_slist*, const char* );
- static void (CURL_EXTERN * slist_free_all) ( curl_slist* );
- static char* (CURL_EXTERN * version) ( void );
- static curl_version_info_data* (CURL_EXTERN * version_info) (CURLversion);
-
- /// A tuple of ASCII function names and pointers to pointers to functions
- static const DYN_LOOKUP dyn_funcs[];
};
#endif // KICAD_CURL_H_
diff --git a/include/kicad_curl/kicad_curl_easy.h b/include/kicad_curl/kicad_curl_easy.h
index 18a3564..80d427c 100644
--- a/include/kicad_curl/kicad_curl_easy.h
+++ b/include/kicad_curl/kicad_curl_easy.h
@@ -89,7 +89,7 @@ public:
void SetHeader( const std::string& aName, const std::string& aValue )
{
std::string header = aName + ':' + aValue;
- m_headers = KICAD_CURL::slist_append( m_headers, header.c_str() );
+ m_headers = curl_slist_append( m_headers, header.c_str() );
}
/**
@@ -150,7 +150,7 @@ public:
*/
const std::string GetErrorText( CURLcode aCode )
{
- return KICAD_CURL::easy_strerror( aCode );
+ return curl_easy_strerror( aCode );
}
/**
@@ -163,7 +163,7 @@ public:
*/
template <typename T> CURLcode SetOption( CURLoption aOption, T aArg )
{
- return KICAD_CURL::easy_setopt( m_CURL, aOption, aArg );
+ return curl_easy_setopt( m_CURL, aOption, aArg );
}
/**
Follow ups
