From 5e591867579c84c03c14a2b785ac6839b72b64aa Mon Sep 17 00:00:00 2001 From: Paul-Louis Ageneau Date: Tue, 26 May 2020 15:32:12 +0200 Subject: [PATCH 1/9] Made certificate generation async to reduce init delay --- include/rtc/peerconnection.hpp | 6 +++- src/certificate.cpp | 51 ++++++++++++++++++++-------------- src/certificate.hpp | 6 +++- src/dtlstransport.cpp | 5 ++-- src/dtlstransport.hpp | 4 +-- src/peerconnection.cpp | 7 +++-- 6 files changed, 49 insertions(+), 30 deletions(-) diff --git a/include/rtc/peerconnection.hpp b/include/rtc/peerconnection.hpp index 334d63a..eea7930 100644 --- a/include/rtc/peerconnection.hpp +++ b/include/rtc/peerconnection.hpp @@ -31,6 +31,7 @@ #include #include +#include #include #include #include @@ -44,6 +45,9 @@ class IceTransport; class DtlsTransport; class SctpTransport; +using certificate_ptr = std::shared_ptr; +using future_certificate_ptr = std::shared_future; + class PeerConnection : public std::enable_shared_from_this { public: enum class State : int { @@ -126,7 +130,7 @@ private: void resetCallbacks(); const Configuration mConfig; - const std::shared_ptr mCertificate; + const future_certificate_ptr mCertificate; std::optional mLocalDescription, mRemoteDescription; mutable std::recursive_mutex mLocalDescriptionMutex, mRemoteDescriptionMutex; diff --git a/src/certificate.cpp b/src/certificate.cpp index 52dcf76..105c035 100644 --- a/src/certificate.cpp +++ b/src/certificate.cpp @@ -141,14 +141,9 @@ string make_fingerprint(gnutls_x509_crt_t crt) { return oss.str(); } -shared_ptr make_certificate(const string &commonName) { - static std::unordered_map> cache; - static std::mutex cacheMutex; - - std::lock_guard lock(cacheMutex); - if (auto it = cache.find(commonName); it != cache.end()) - return it->second; +namespace { +certificate_ptr make_certificate_impl(string commonName) { std::unique_ptr crt(create_crt(), delete_crt); std::unique_ptr privkey(create_privkey(), delete_privkey); @@ -174,11 +169,11 @@ shared_ptr make_certificate(const string &commonName) { check_gnutls(gnutls_x509_crt_sign2(*crt, *crt, *privkey, GNUTLS_DIG_SHA256, 0), "Unable to auto-sign certificate"); - auto certificate = std::make_shared(*crt, *privkey); - cache.emplace(std::make_pair(commonName, certificate)); - return certificate; + return std::make_shared(*crt, *privkey); } +} // namespace + } // namespace rtc #else @@ -236,15 +231,9 @@ string make_fingerprint(X509 *x509) { return oss.str(); } +namespace { -shared_ptr make_certificate(const string &commonName) { - static std::unordered_map> cache; - static std::mutex cacheMutex; - - std::lock_guard lock(cacheMutex); - if (auto it = cache.find(commonName); it != cache.end()) - return it->second; - +certificate_ptr make_certificate_impl(string commonName) { shared_ptr x509(X509_new(), X509_free); shared_ptr pkey(EVP_PKEY_new(), EVP_PKEY_free); @@ -281,12 +270,32 @@ shared_ptr make_certificate(const string &commonName) { if (!X509_sign(x509.get(), pkey.get(), EVP_sha256())) throw std::runtime_error("Unable to auto-sign certificate"); - auto certificate = std::make_shared(x509, pkey); - cache.emplace(std::make_pair(commonName, certificate)); - return certificate; + return std::make_shared(x509, pkey); } +} // namespace + } // namespace rtc #endif +// Common for GnuTLS and OpenSSL + +namespace rtc { + +future_certificate_ptr make_certificate(string commonName) { + static std::unordered_map cache; + static std::mutex cacheMutex; + + std::lock_guard lock(cacheMutex); + + if (auto it = cache.find(commonName); it != cache.end()) + return it->second; + + auto future = std::async(make_certificate_impl, commonName); + auto shared = future.share(); + cache.emplace(std::move(commonName), shared); + return shared; +} + +} // namespace rtc diff --git a/src/certificate.hpp b/src/certificate.hpp index cda2fb5..b20f415 100644 --- a/src/certificate.hpp +++ b/src/certificate.hpp @@ -21,6 +21,7 @@ #include "include.hpp" +#include #include #if USE_GNUTLS @@ -62,7 +63,10 @@ string make_fingerprint(gnutls_x509_crt_t crt); string make_fingerprint(X509 *x509); #endif -std::shared_ptr make_certificate(const string &commonName); +using certificate_ptr = std::shared_ptr; +using future_certificate_ptr = std::shared_future; + +future_certificate_ptr make_certificate(string commonName); } // namespace rtc diff --git a/src/dtlstransport.cpp b/src/dtlstransport.cpp index 7723130..8d7d842 100644 --- a/src/dtlstransport.cpp +++ b/src/dtlstransport.cpp @@ -63,9 +63,8 @@ void DtlsTransport::Cleanup() { // Nothing to do } -DtlsTransport::DtlsTransport(shared_ptr lower, shared_ptr certificate, - verifier_callback verifierCallback, - state_callback stateChangeCallback) +DtlsTransport::DtlsTransport(shared_ptr lower, certificate_ptr certificate, + verifier_callback verifierCallback, state_callback stateChangeCallback) : Transport(lower), mCertificate(certificate), mState(State::Disconnected), mVerifierCallback(std::move(verifierCallback)), mStateChangeCallback(std::move(stateChangeCallback)) { diff --git a/src/dtlstransport.hpp b/src/dtlstransport.hpp index d0e6030..c8200b9 100644 --- a/src/dtlstransport.hpp +++ b/src/dtlstransport.hpp @@ -51,7 +51,7 @@ public: using verifier_callback = std::function; using state_callback = std::function; - DtlsTransport(std::shared_ptr lower, std::shared_ptr certificate, + DtlsTransport(std::shared_ptr lower, certificate_ptr certificate, verifier_callback verifierCallback, state_callback stateChangeCallback); ~DtlsTransport(); @@ -65,7 +65,7 @@ private: void changeState(State state); void runRecvLoop(); - const std::shared_ptr mCertificate; + const certificate_ptr mCertificate; Queue mIncomingQueue; std::atomic mState; diff --git a/src/peerconnection.cpp b/src/peerconnection.cpp index 234b3eb..82794b6 100644 --- a/src/peerconnection.cpp +++ b/src/peerconnection.cpp @@ -269,9 +269,10 @@ shared_ptr PeerConnection::initDtlsTransport() { if (auto transport = std::atomic_load(&mDtlsTransport)) return transport; + auto certificate = mCertificate.get(); auto lower = std::atomic_load(&mIceTransport); auto transport = std::make_shared( - lower, mCertificate, weak_bind_verifier(&PeerConnection::checkFingerprint, this, _1), + lower, certificate, weak_bind_verifier(&PeerConnection::checkFingerprint, this, _1), [this, weak_this = weak_from_this()](DtlsTransport::State state) { auto shared_this = weak_this.lock(); if (!shared_this) @@ -513,9 +514,11 @@ void PeerConnection::processLocalDescription(Description description) { if (auto remote = remoteDescription()) remoteSctpPort = remote->sctpPort(); + auto certificate = mCertificate.get(); // wait for certificate if not ready + std::lock_guard lock(mLocalDescriptionMutex); mLocalDescription.emplace(std::move(description)); - mLocalDescription->setFingerprint(mCertificate->fingerprint()); + mLocalDescription->setFingerprint(certificate->fingerprint()); mLocalDescription->setSctpPort(remoteSctpPort.value_or(DEFAULT_SCTP_PORT)); mLocalDescription->setMaxMessageSize(LOCAL_MAX_MESSAGE_SIZE); From 7a2e0c267ddfd1bc2126bd3205059c076ae34003 Mon Sep 17 00:00:00 2001 From: Paul-Louis Ageneau Date: Tue, 26 May 2020 17:42:03 +0200 Subject: [PATCH 2/9] Set flag CMAKE_THREAD_PREFER_PTHREAD --- CMakeLists.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 9a3d81f..47e0430 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -71,7 +71,8 @@ set(TESTS_ANSWERER_SOURCES ${CMAKE_CURRENT_SOURCE_DIR}/test/p2p/answerer.cpp ) -set(THREADS_PREFER_PTHREAD_FLAG ON) +set(CMAKE_THREAD_PREFER_PTHREAD TRUE) +set(THREADS_PREFER_PTHREAD_FLAG TRUE) find_package(Threads REQUIRED) add_subdirectory(deps/usrsctp EXCLUDE_FROM_ALL) From 5ae311c50a2f61ee922c5fb9a2e5ce5abc19cd6f Mon Sep 17 00:00:00 2001 From: Paul-Louis Ageneau Date: Tue, 26 May 2020 17:56:49 +0200 Subject: [PATCH 3/9] Made dependency on Threads public --- CMakeLists.txt | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 47e0430..6022b94 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -93,10 +93,11 @@ set_target_properties(datachannel PROPERTIES CXX_STANDARD 17) target_include_directories(datachannel PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/include) +target_include_directories(datachannel PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/deps/plog/include) target_include_directories(datachannel PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/include/rtc) target_include_directories(datachannel PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/src) -target_include_directories(datachannel PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/deps/plog/include) -target_link_libraries(datachannel Threads::Threads Usrsctp::UsrsctpStatic) +target_link_libraries(datachannel PUBLIC Threads::Threads) +target_link_libraries(datachannel PRIVATE Usrsctp::UsrsctpStatic) add_library(datachannel-static STATIC EXCLUDE_FROM_ALL ${LIBDATACHANNEL_SOURCES}) set_target_properties(datachannel-static PROPERTIES @@ -104,14 +105,15 @@ set_target_properties(datachannel-static PROPERTIES CXX_STANDARD 17) target_include_directories(datachannel-static PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/include) +target_include_directories(datachannel-static PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/deps/plog/include) target_include_directories(datachannel-static PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/include/rtc) target_include_directories(datachannel-static PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/src) -target_include_directories(datachannel-static PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/deps/plog/include) -target_link_libraries(datachannel-static Threads::Threads Usrsctp::UsrsctpStatic) +target_link_libraries(datachannel-static PUBLIC Threads::Threads) +target_link_libraries(datachannel-static PRIVATE Usrsctp::UsrsctpStatic) if(WIN32) - target_link_libraries(datachannel "wsock32" "ws2_32") # winsock2 - target_link_libraries(datachannel-static "wsock32" "ws2_32") # winsock2 + target_link_libraries(datachannel PRIVATE wsock32 ws2_32) # winsock2 + target_link_libraries(datachannel-static PRIVATE wsock32 ws2_32) # winsock2 endif() if (USE_GNUTLS) @@ -125,29 +127,29 @@ if (USE_GNUTLS) IMPORTED_LOCATION "${GNUTLS_LIBRARIES}") endif() target_compile_definitions(datachannel PRIVATE USE_GNUTLS=1) - target_link_libraries(datachannel GnuTLS::GnuTLS) + target_link_libraries(datachannel PRIVATE GnuTLS::GnuTLS) target_compile_definitions(datachannel-static PRIVATE USE_GNUTLS=1) - target_link_libraries(datachannel-static GnuTLS::GnuTLS) + target_link_libraries(datachannel-static PRIVATE GnuTLS::GnuTLS) else() find_package(OpenSSL REQUIRED) target_compile_definitions(datachannel PRIVATE USE_GNUTLS=0) - target_link_libraries(datachannel OpenSSL::SSL) + target_link_libraries(datachannel PRIVATE OpenSSL::SSL) target_compile_definitions(datachannel-static PRIVATE USE_GNUTLS=0) - target_link_libraries(datachannel-static OpenSSL::SSL) + target_link_libraries(datachannel-static PRIVATE OpenSSL::SSL) endif() if (USE_JUICE) add_subdirectory(deps/libjuice EXCLUDE_FROM_ALL) target_compile_definitions(datachannel PRIVATE USE_JUICE=1) - target_link_libraries(datachannel LibJuice::LibJuiceStatic) + target_link_libraries(datachannel PRIVATE LibJuice::LibJuiceStatic) target_compile_definitions(datachannel-static PRIVATE USE_JUICE=1) - target_link_libraries(datachannel-static LibJuice::LibJuiceStatic) + target_link_libraries(datachannel-static PRIVATE LibJuice::LibJuiceStatic) else() find_package(LibNice REQUIRED) target_compile_definitions(datachannel PRIVATE USE_JUICE=0) - target_link_libraries(datachannel LibNice::LibNice) + target_link_libraries(datachannel PRIVATE LibNice::LibNice) target_compile_definitions(datachannel-static PRIVATE USE_JUICE=0) - target_link_libraries(datachannel-static LibNice::LibNice) + target_link_libraries(datachannel-static PRIVATE LibNice::LibNice) endif() add_library(LibDataChannel::LibDataChannel ALIAS datachannel) From 3be2b0427f941ef6f0e96f1273aab368a5db16c1 Mon Sep 17 00:00:00 2001 From: Paul-Louis Ageneau Date: Tue, 26 May 2020 21:57:05 +0200 Subject: [PATCH 4/9] Replaced std::async call with custom thread invocation --- src/certificate.cpp | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/src/certificate.cpp b/src/certificate.cpp index 105c035..c160bd1 100644 --- a/src/certificate.cpp +++ b/src/certificate.cpp @@ -281,6 +281,23 @@ certificate_ptr make_certificate_impl(string commonName) { // Common for GnuTLS and OpenSSL +namespace { + +// Helper function roughly equivalent to std::async with policy std::launch::async +// since std::async might be unreliable on some platforms (e.g. Mingw32 on Windows) +template +std::future(std::decay_t...)>> thread_call(F &&f, + Args &&... args) { + using R = std::result_of_t(std::decay_t...)>; + std::packaged_task task(std::bind(f, std::forward(args)...)); + std::future future = task.get_future(); + std::thread t(std::move(task)); + t.detach(); + return future; +} + +} // namespace + namespace rtc { future_certificate_ptr make_certificate(string commonName) { @@ -292,7 +309,7 @@ future_certificate_ptr make_certificate(string commonName) { if (auto it = cache.find(commonName); it != cache.end()) return it->second; - auto future = std::async(make_certificate_impl, commonName); + auto future = thread_call(make_certificate_impl, commonName); auto shared = future.share(); cache.emplace(std::move(commonName), shared); return shared; From 96501a8a68841655257bd4e3e256cba0a4b29977 Mon Sep 17 00:00:00 2001 From: Paul-Louis Ageneau Date: Wed, 27 May 2020 10:32:29 +0200 Subject: [PATCH 5/9] Clear certificate cache on cleanup --- src/certificate.cpp | 21 +++++++++++++-------- src/certificate.hpp | 4 +++- src/init.cpp | 2 ++ 3 files changed, 18 insertions(+), 9 deletions(-) diff --git a/src/certificate.cpp b/src/certificate.cpp index c160bd1..ca35734 100644 --- a/src/certificate.cpp +++ b/src/certificate.cpp @@ -281,6 +281,8 @@ certificate_ptr make_certificate_impl(string commonName) { // Common for GnuTLS and OpenSSL +namespace rtc { + namespace { // Helper function roughly equivalent to std::async with policy std::launch::async @@ -296,23 +298,26 @@ std::future(std::decay_t...)>> thread_cal return future; } +static std::unordered_map CertificateCache; +static std::mutex CertificateCacheMutex; + } // namespace -namespace rtc { - future_certificate_ptr make_certificate(string commonName) { - static std::unordered_map cache; - static std::mutex cacheMutex; + std::lock_guard lock(CertificateCacheMutex); - std::lock_guard lock(cacheMutex); - - if (auto it = cache.find(commonName); it != cache.end()) + if (auto it = CertificateCache.find(commonName); it != CertificateCache.end()) return it->second; auto future = thread_call(make_certificate_impl, commonName); auto shared = future.share(); - cache.emplace(std::move(commonName), shared); + CertificateCache.emplace(std::move(commonName), shared); return shared; } +void CleanupCertificateCache() { + std::lock_guard lock(CertificateCacheMutex); + CertificateCache.clear(); +} + } // namespace rtc diff --git a/src/certificate.hpp b/src/certificate.hpp index b20f415..fd5affb 100644 --- a/src/certificate.hpp +++ b/src/certificate.hpp @@ -66,7 +66,9 @@ string make_fingerprint(X509 *x509); using certificate_ptr = std::shared_ptr; using future_certificate_ptr = std::shared_future; -future_certificate_ptr make_certificate(string commonName); +future_certificate_ptr make_certificate(string commonName); // cached + +void CleanupCertificateCache(); } // namespace rtc diff --git a/src/init.cpp b/src/init.cpp index 38afc1a..2fbfcea 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -18,6 +18,7 @@ #include "init.hpp" +#include "certificate.hpp" #include "dtlstransport.hpp" #include "sctptransport.hpp" @@ -74,6 +75,7 @@ Init::Init() { } Init::~Init() { + CleanupCertificateCache(); DtlsTransport::Cleanup(); SctpTransport::Cleanup(); From f8df667a14e8a9134039c6fb7c91d1cddc70d3ee Mon Sep 17 00:00:00 2001 From: Paul-Louis Ageneau Date: Wed, 27 May 2020 11:04:34 +0200 Subject: [PATCH 6/9] Created separate workflows for different backends --- .../workflows/{build.yml => build-gnutls.yml} | 13 ++++++---- .github/workflows/build-nice.yml | 24 +++++++++++++++++++ .github/workflows/build-openssl.yml | 24 +++++++++++++++++++ 3 files changed, 56 insertions(+), 5 deletions(-) rename .github/workflows/{build.yml => build-gnutls.yml} (78%) create mode 100644 .github/workflows/build-nice.yml create mode 100644 .github/workflows/build-openssl.yml diff --git a/.github/workflows/build.yml b/.github/workflows/build-gnutls.yml similarity index 78% rename from .github/workflows/build.yml rename to .github/workflows/build-gnutls.yml index 43a92f9..77d804e 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build-gnutls.yml @@ -1,11 +1,14 @@ -name: Build and test -on: [push, pull_request] - +name: Build and test with GnuTLS +on: + push: + branches: + - master + pull_request: + branches: + - master jobs: build: - runs-on: ubuntu-latest - steps: - uses: actions/checkout@v2 - name: install packages diff --git a/.github/workflows/build-nice.yml b/.github/workflows/build-nice.yml new file mode 100644 index 0000000..362c262 --- /dev/null +++ b/.github/workflows/build-nice.yml @@ -0,0 +1,24 @@ +name: Build and test with libnice +on: + push: + branches: + - master + pull_request: + branches: + - master +jobs: + build: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + - name: install packages + run: sudo apt update && sudo apt install libgnutls28-dev libnice-dev + - name: submodules + run: git submodule update --init --recursive + - name: cmake + run: cmake -B build -DUSE_JUICE=0 -DUSE_GNUTLS=1 + - name: make + run: (cd build; make) + - name: test + run: ./build/tests + diff --git a/.github/workflows/build-openssl.yml b/.github/workflows/build-openssl.yml new file mode 100644 index 0000000..999bbab --- /dev/null +++ b/.github/workflows/build-openssl.yml @@ -0,0 +1,24 @@ +name: Build and test with OpenSSL +on: + push: + branches: + - master + pull_request: + branches: + - master +jobs: + build: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + - name: install packages + run: sudo apt update && sudo apt install libssl-dev + - name: submodules + run: git submodule update --init --recursive + - name: cmake + run: cmake -B build -DUSE_JUICE=1 -DUSE_GNUTLS=0 + - name: make + run: (cd build; make) + - name: test + run: ./build/tests + From b3bba4286b365d18ad0fa678f410c0e985c4cc6b Mon Sep 17 00:00:00 2001 From: Paul-Louis Ageneau Date: Wed, 27 May 2020 11:05:48 +0200 Subject: [PATCH 7/9] Updated libjuice --- deps/libjuice | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/deps/libjuice b/deps/libjuice index e56531c..3e91d38 160000 --- a/deps/libjuice +++ b/deps/libjuice @@ -1 +1 @@ -Subproject commit e56531c647b157e0ea93368b8ec4b635895d3b57 +Subproject commit 3e91d3869ba089423273e15b4277358b15a77950 From 9436757f73a9d76c9d4571d23a73826e0f56b494 Mon Sep 17 00:00:00 2001 From: Paul-Louis Ageneau Date: Wed, 27 May 2020 11:25:21 +0200 Subject: [PATCH 8/9] Updated libjuice to v0.4.0 --- deps/libjuice | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/deps/libjuice b/deps/libjuice index 3e91d38..511b19f 160000 --- a/deps/libjuice +++ b/deps/libjuice @@ -1 +1 @@ -Subproject commit 3e91d3869ba089423273e15b4277358b15a77950 +Subproject commit 511b19f5168825f1d72e79bee4aef8f3b5a36fca From 22e71bd663191e8aa0c0604eb5e56f909f937c34 Mon Sep 17 00:00:00 2001 From: Paul-Louis Ageneau Date: Wed, 27 May 2020 11:26:11 +0200 Subject: [PATCH 9/9] Bumped version to 0.5.0 --- CMakeLists.txt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 6022b94..235412a 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1,7 +1,7 @@ cmake_minimum_required (VERSION 3.7) project (libdatachannel DESCRIPTION "WebRTC DataChannels Library" - VERSION 0.4.9 + VERSION 0.5.0 LANGUAGES CXX) option(USE_GNUTLS "Use GnuTLS instead of OpenSSL" OFF) @@ -19,8 +19,8 @@ set(CMAKE_MODULE_PATH ${PROJECT_SOURCE_DIR}/cmake/Modules) if(WIN32) add_definitions(-DWIN32_LEAN_AND_MEAN) if (MSVC) - add_definitions(-DNOMINMAX) - endif() + add_definitions(-DNOMINMAX) + endif() endif() set(LIBDATACHANNEL_SOURCES