From cc5723ee68a75c6d1d80c7ecc3f23c432c65a64f Mon Sep 17 00:00:00 2001 From: turtledreams <62231246+turtledreams@users.noreply.github.com> Date: Wed, 5 Aug 2026 11:46:12 +0900 Subject: [PATCH 1/5] multi support --- CHANGELOG.md | 7 + CMakeLists.txt | 3 +- include/countly.hpp | 97 ++++++- include/countly/constants.hpp | 21 +- include/countly/path_utils.hpp | 97 +++++++ include/countly/request_module.hpp | 18 ++ python_build_script.py | 125 +++++---- src/configuration_module.cpp | 2 +- src/countly.cpp | 287 ++++++++++++++++++-- src/request_module.cpp | 76 +++++- tests/multi_instance.cpp | 402 +++++++++++++++++++++++++++++ tests/test_utils.hpp | 92 ++++++- 12 files changed, 1139 insertions(+), 88 deletions(-) create mode 100644 include/countly/path_utils.hpp create mode 100644 tests/multi_instance.cpp diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a480753..d34bbb84 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,13 @@ ## X.X.X - ! Minor breaking change ! Added SDK internal limits enforcement (max key length, value size, segmentation values, breadcrumb count, stack-trace lines per thread, stack-trace line length) across events, views, crashes, and user properties. Limits use config defaults overridable by server SDK Behavior Settings, and can be set via `setMaxKeyLength`, `setMaxValueSize`, `setMaxSegmentationValues`, `setMaxBreadcrumbCount`, `setMaxStackTraceLinesPerThread`, `setMaxStackTraceLineLength` during init. +- Added multi-instance support: several `Countly` instances can now run in one process, each with its own app key, queues, storage, and threads. Instances can be owned by the integrator, or created and looked up by name with `createInstance`, `getInstance(name)`, `findInstance`, `hasInstance`, `destroyInstance` and `destroyAllInstances`. +- ! Minor breaking change ! When built with SQLite, each instance requires its own database path. A second instance claiming a path already in use logs an error and does not initialize. +- Fixed the libcurl global lifecycle: `curl_global_init` now runs once per process, and cleanup no longer runs when an instance is destroyed, which could tear down networking underneath another live instance. Added `shutdownNetworking()` for hosts that load and unload the SDK without exiting. +- Fixed non-unique event and view IDs: the random component of generated IDs was constant for the lifetime of the process, and on platforms with a coarse `system_clock` (Windows, ~15ms) the timestamp component did not change either, so IDs generated within one tick were identical. +- Remote config fetches now run on owned threads that are joined by `stop()` and by destruction, instead of being detached. Consecutive remote config calls block until the previous fetch completes. +- `stop()` now also stops periodic SDK Behavior Settings updates. + ## 26.1.1 - Updated CMake minimum required version to use the range format with upper the end of `3.31`. - Hardened mutex handling against exceptions. diff --git a/CMakeLists.txt b/CMakeLists.txt index 867b0378..ecb28e74 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -122,7 +122,8 @@ if(COUNTLY_BUILD_TESTS) ${CMAKE_CURRENT_SOURCE_DIR}/tests/mutex_exception_safety.cpp ${CMAKE_CURRENT_SOURCE_DIR}/tests/sbs.cpp ${CMAKE_CURRENT_SOURCE_DIR}/tests/internal_limits.cpp - ${CMAKE_CURRENT_SOURCE_DIR}/tests/internal_limits_integration.cpp) + ${CMAKE_CURRENT_SOURCE_DIR}/tests/internal_limits_integration.cpp + ${CMAKE_CURRENT_SOURCE_DIR}/tests/multi_instance.cpp) target_compile_options(countly-tests PRIVATE -g) target_compile_definitions(countly-tests PRIVATE COUNTLY_BUILD_TESTS) diff --git a/include/countly.hpp b/include/countly.hpp index 6f9dbc79..60016b66 100644 --- a/include/countly.hpp +++ b/include/countly.hpp @@ -14,6 +14,7 @@ #include #include #include +#include #include @@ -39,15 +40,73 @@ class Countly : public cly::CountlyDelegates { virtual ~Countly(); - // Returns the singleton instance of Countly + // Returns the default instance, creating it on first use. static Countly &getInstance(); - // Do not implicitly generate the copy constructor, this is a singleton. + /** + * Returns the instance registered under 'name', creating an uninitialized one + * and logging a WARNING if that name is not registered yet. The empty name is + * the default instance, so getInstance("") == getInstance(). + * + * The returned reference is valid until that instance is destroyed by + * destroyInstance(), destroyAllInstances() or halt(). + */ + static Countly &getInstance(const std::string &name); + + /** + * Creates and returns the instance registered under 'name'. If that name is + * already registered, returns the existing instance and logs a WARNING. + */ + static Countly &createInstance(const std::string &name); + + /** + * Returns the instance registered under 'name', or nullptr if there is none. + * Unlike getInstance(name) this never creates anything, and unlike + * hasInstance() followed by getInstance() it is a single lookup, so no other + * thread can destroy the instance in between. + */ + static Countly *findInstance(const std::string &name); + + /** + * @return true if an instance is registered under 'name'. + */ + static bool hasInstance(const std::string &name); + + /** + * Destroys the instance registered under 'name'. Joins its threads, ends its + * session and frees its database path claim. A no-op if the name is not + * registered. Any reference or pointer to that instance dangles afterwards. + */ + static void destroyInstance(const std::string &name); + + /** + * Destroys every registered instance, in reverse creation order. + */ + static void destroyAllInstances(); + + /** + * Releases process-wide networking state (libcurl's global data). Optional: + * process exit does this automatically. Call it only after destroying every + * instance, and only in a host that loads and unloads the SDK without exiting + * -- a plugin host using dlopen/dlclose, for example. Logs an error and does + * nothing if any instance is still live. + * + * A no-op on builds that do not use libcurl (Windows/WinHTTP, + * COUNTLY_USE_CUSTOM_HTTP). + */ + static void shutdownNetworking(); + + // Do not implicitly generate the copy constructor, instances are not copyable. Countly(const Countly &) = delete; - // Do not implicitly generate the copy assignment operator, this is a singleton. + // Do not implicitly generate the copy assignment operator. void operator=(const Countly &) = delete; + // Not movable either: modules hold `CountlyDelegates *this` and background + // threads capture `this`, so relocating an instance is unsound. + Countly(Countly &&) = delete; + Countly &operator=(Countly &&) = delete; + void alwaysUsePost(bool value); void setMaxRequestQueueSize(unsigned int requestQueueSize); @@ -355,9 +414,33 @@ class Countly : public cly::CountlyDelegates { inline const CountlyConfiguration &getConfiguration() { return *configuration.get(); } static void halt(); + + static size_t debugClaimedPathCount(); + + static int debugLiveInstanceCount(); #endif private: + /** + * Copies the default instance's logger callback onto a freshly created + * instance. Without this, a WARNING logged on a brand-new instance is + * swallowed: LoggerModule::log() no-ops when no callback is set. + */ + static void inheritDefaultLogger(Countly &instance); + + /** + * Gives back this instance's database path claim. A no-op when no claim is + * held, and compiled to nothing on non-SQLite builds. + */ + void releaseDatabasePathClaim(); + + /** + * Joins and clears every remote-config fetch thread. Safe to call repeatedly + * and safe to call when none are running. Must not be called while the + * instance mutex is held -- the fetch threads take it. + */ + void joinRemoteConfigThreads(); + void _deleteThread(); void _sendIndependantLocationRequest(); void log(LogLevel level, const std::string &message); @@ -391,6 +474,11 @@ class Countly : public cly::CountlyDelegates { nlohmann::json session_params; std::unique_ptr thread; + + // Remote-config fetches run on owned threads rather than detached ones: a + // detached thread captures `this` and can outlive the instance. + std::mutex remote_config_thread_mutex; + std::vector remote_config_threads; std::unique_ptr crash_module; std::unique_ptr views_module; @@ -414,6 +502,9 @@ class Countly : public cly::CountlyDelegates { std::deque event_queue; #else std::string database_path; + // The normalized database path this instance has claimed in the process-wide + // registry, empty when it holds no claim. + std::string claimed_database_path; #endif bool remote_config_enabled = false; diff --git a/include/countly/constants.hpp b/include/countly/constants.hpp index 622e723b..e7bbe7f6 100644 --- a/include/countly/constants.hpp +++ b/include/countly/constants.hpp @@ -11,6 +11,7 @@ #include #include #include +#include #define COUNTLY_SDK_NAME "cpp-native-unknown" #define COUNTLY_SDK_VERSION "26.1.1" @@ -34,8 +35,6 @@ struct HTTPResponse { using HTTPClientFunction = std::function; using SHA256Function = std::function; namespace utils { -const std::default_random_engine generator(std::chrono::system_clock::now().time_since_epoch().count()); -const std::uniform_int_distribution distribution(1, INT_MAX); /** * Formats the given arguments into a string buffer. @@ -68,13 +67,21 @@ static std::string mapToString(const std::map &m) { return std::to_string(lenght); } /** - * Generate a random UUID. + * Generate an event/view ID. * - * @return a string object holding a UUID. + * The engine is thread_local and is *advanced* across calls. An earlier version + * bound a copy of a const engine on each call, which meant every ID in the + * process shared one random component; on platforms with a coarse system_clock + * (Windows, ~15ms) the timestamp did not move either, so IDs generated inside + * one tick were identical. + * + * @return a string object holding the ID. */ -static std::string generateEventID() { - auto dice = std::bind(distribution, generator); - int random = dice(); +inline std::string generateEventID() { + static thread_local std::mt19937 engine(static_cast(std::chrono::system_clock::now().time_since_epoch().count() ^ static_cast(std::hash()(std::this_thread::get_id())))); + static thread_local std::uniform_int_distribution distribution(1, INT_MAX); + + const int random = distribution(engine); std::chrono::system_clock::time_point now = std::chrono::system_clock::now(); const auto timestamp = now.time_since_epoch(); diff --git a/include/countly/path_utils.hpp b/include/countly/path_utils.hpp new file mode 100644 index 00000000..602b107c --- /dev/null +++ b/include/countly/path_utils.hpp @@ -0,0 +1,97 @@ +#ifndef COUNTLY_PATH_UTILS_HPP_ +#define COUNTLY_PATH_UTILS_HPP_ + +#include +#include + +namespace cly { +namespace utils { + +/** + * Lexically normalizes a database path so two spellings of the same path + * compare equal. Purely textual: it never touches the filesystem, so it works + * for a database file that does not exist yet. + * + * This is a guardrail, not a boundary. It does not resolve symlinks, + * hardlinks, SQLite URI filenames (`file:a.db?mode=rwc`) or Windows 8.3 short + * names. + */ +inline std::string normalizeDatabasePath(const std::string &path) { + // 1. Trim surrounding whitespace. + static const char *const WHITESPACE = " \t\n\r\f\v"; + const std::string::size_type first = path.find_first_not_of(WHITESPACE); + if (first == std::string::npos) { + return ""; + } + const std::string::size_type last = path.find_last_not_of(WHITESPACE); + std::string working = path.substr(first, last - first + 1); + + // 2. Unify separators. + for (std::string::size_type index = 0; index < working.size(); ++index) { + if (working[index] == '\\') { + working[index] = '/'; + } + } + + // 3. Split off a root prefix so that "//host/share" and "/var" survive the + // empty-segment collapse below. + std::string prefix; + if (working.compare(0, 2, "//") == 0) { + prefix = "//"; + working = working.substr(2); + } else if (!working.empty() && working[0] == '/') { + prefix = "/"; + working = working.substr(1); + } + + // 4 + 5. Walk the segments: drop empties (collapsed separators, trailing + // slash) and ".", resolve ".." lexically. + std::vector segments; + std::string::size_type start = 0; + while (true) { + const std::string::size_type slash = working.find('/', start); + const std::string segment = (slash == std::string::npos) ? working.substr(start) : working.substr(start, slash - start); + + if (segment.empty() || segment == ".") { + // nothing to add + } else if (segment == "..") { + if (!segments.empty() && segments.back() != "..") { + segments.pop_back(); + } else if (prefix.empty()) { + segments.push_back(segment); // a leading ".." on a relative path is meaningful + } + // ".." at the root of an absolute path has nowhere to go; drop it + } else { + segments.push_back(segment); + } + + if (slash == std::string::npos) { + break; + } + start = slash + 1; + } + + // 6. Rejoin. + std::string result = prefix; + for (std::vector::size_type index = 0; index < segments.size(); ++index) { + if (index > 0) { + result += "/"; + } + result += segments[index]; + } + + // 7. Case-insensitive filesystem. +#ifdef _WIN32 + for (std::string::size_type index = 0; index < result.size(); ++index) { + if (result[index] >= 'A' && result[index] <= 'Z') { + result[index] = static_cast(result[index] - 'A' + 'a'); + } + } +#endif + + return result; +} + +} // namespace utils +} // namespace cly +#endif diff --git a/include/countly/request_module.hpp b/include/countly/request_module.hpp index 3c5087c3..c811ac6e 100644 --- a/include/countly/request_module.hpp +++ b/include/countly/request_module.hpp @@ -39,6 +39,24 @@ class RequestModule { long long RQSize(); void setConfigurationProvider(std::weak_ptr provider); // try injecting + /** + * Process-wide network initialisation. Idempotent, and a no-op unless the SDK + * is built against libcurl. + */ + static void initGlobalNetworking(); + + /** + * Process-wide network teardown. Idempotent, and a no-op unless the SDK is + * built against libcurl. Must not be called while any instance is live -- + * Countly::shutdownNetworking() enforces that. + */ + static void releaseGlobalNetworking(); + +#ifdef COUNTLY_BUILD_TESTS + static int globalNetworkingInitCount(); + static bool globalNetworkingReleased(); +#endif + private: class RequestModuleImpl; std::unique_ptr impl; diff --git a/python_build_script.py b/python_build_script.py index 6d651202..a26a94fc 100755 --- a/python_build_script.py +++ b/python_build_script.py @@ -1,66 +1,97 @@ +#!/usr/bin/env python3 import os +import sys +import shutil +import subprocess import datetime import logging -import itertools -# Set up logging +# Setup log_filename = 'test_results_{}.log'.format(datetime.datetime.now().strftime('%Y%m%d_%H%M%S')) logging.basicConfig(filename=log_filename, level=logging.INFO, format='%(asctime)s:%(levelname)s:%(message)s') +console = logging.StreamHandler() +console.setLevel(logging.INFO) +logging.getLogger().addHandler(console) + +is_windows = os.name == 'nt' +repo_root = os.path.abspath(os.getcwd()) -# List all combinations of COUNTLY_USE_CUSTOM_SHA256 and COUNTLY_USE_SQLITE options = [(0, 0), (0, 1), (1, 0), (1, 1)] +build_dir = os.path.join(repo_root, 'build') +config = 'Debug' # change to Release if needed for custom_sha256, use_sqlite in options: - print("Running script with COUNTLY_USE_CUSTOM_SHA256="+str(custom_sha256)+" and COUNTLY_USE_SQLITE="+str(use_sqlite)) - logging.info("Running script with COUNTLY_USE_CUSTOM_SHA256="+str(custom_sha256)+" and COUNTLY_USE_SQLITE="+str(use_sqlite)) - + cfg_name = f'custom_sha256={custom_sha256}, sqlite={use_sqlite}' + print(f"\n=== Running configuration: {cfg_name} ===") + logging.info("Running configuration: %s", cfg_name) - # Check if script is in the "build" folder - if os.path.basename(os.getcwd()) == "build": - print("In build folder. Changing directory...") - logging.info("In build folder. Changing directory...") - os.chdir("..") - print("Current directory:", os.getcwd()) - logging.info("Current directory: {}".format(os.getcwd())) - else: - print("Not in build folder. Current directory:", os.getcwd()) - logging.info("Not in build folder. Current directory: {}".format(os.getcwd())) + try: + # Ensure we're at repo root + if os.path.basename(os.getcwd()) == 'build': + os.chdir('..') - # Delete "build" folder and subfolders - print("Deleting folder and its subfolders...") - logging.info("Deleting folder and its subfolders...") - os.system("rm -rf build") - print("Folder and subfolders deleted.") - logging.info("Folder and subfolders deleted.") + # Remove build directory + if os.path.isdir(build_dir): + logging.info("Removing existing build directory: %s", build_dir) + shutil.rmtree(build_dir) + os.makedirs(build_dir, exist_ok=True) + # Configure with CMake + cmake_args = [ + 'cmake', + '-DCOUNTLY_BUILD_SAMPLE=1', + '-DCOUNTLY_BUILD_TESTS=1', + f'-DCOUNTLY_USE_CUSTOM_SHA256={custom_sha256}', + f'-DCOUNTLY_USE_SQLITE={use_sqlite}', + '-B', build_dir, + repo_root + ] + logging.info("Running cmake configure: %s", ' '.join(cmake_args)) + subprocess.run(cmake_args, check=True) - # Run cmake to generate makefiles - build_dir = "build" - cmake_command = "cmake -DCOUNTLY_BUILD_SAMPLE=1 -DCOUNTLY_BUILD_TESTS=1 -DCOUNTLY_USE_CUSTOM_SHA256={} -DCOUNTLY_USE_SQLITE={} -B {} .".format(custom_sha256, use_sqlite, build_dir) - os.system(cmake_command) - logging.info("Ran cmake to generate makefiles.") + # Build sample and tests via cmake --build (cross platform) + build_sample_cmd = ['cmake', '--build', build_dir, '--config', config, '--target', 'countly-sample'] + build_tests_cmd = ['cmake', '--build', build_dir, '--config', config, '--target', 'countly-tests'] + logging.info("Building sample: %s", ' '.join(build_sample_cmd)) + subprocess.run(build_sample_cmd, check=True) - # Change directory to "build" and build the sample and tests - os.chdir("build") - print("Current directory:", os.getcwd()) - logging.info("Current directory: {}".format(os.getcwd())) - os.system("make ./countly-sample") - os.system("make ./countly-tests") + logging.info("Building tests: %s", ' '.join(build_tests_cmd)) + subprocess.run(build_tests_cmd, check=False) # tests might be absent depending on CMake options - # Redirect standard output and standard error to a file - output_file = "doctest_results.txt" - with open(output_file, "w") as f: - os.system("./countly-tests > {} 2>&1".format(output_file)) + # Run tests: prefer ctest, fallback to running test executable + output_file = os.path.join(build_dir, f'doctest_results_{custom_sha256}_{use_sqlite}.txt') + try: + # Try ctest first + ctest_cmd = ['ctest', '--test-dir', build_dir, '-C', config, '--output-on-failure'] + logging.info("Running ctest: %s", ' '.join(ctest_cmd)) + with open(output_file, 'w', encoding='utf-8') as out: + completed = subprocess.run(ctest_cmd, stdout=out, stderr=subprocess.STDOUT, check=False) + logging.info("ctest exit code: %s", completed.returncode) + except FileNotFoundError: + # ctest not available: try to run the test binary directly + logging.info("ctest not available; attempting to run test binary directly") + test_bin_name = 'countly-tests.exe' if is_windows else 'countly-tests' + test_path = os.path.join(build_dir, config, test_bin_name) if is_windows else os.path.join(build_dir, test_bin_name) + if os.path.exists(test_path): + with open(output_file, 'w', encoding='utf-8') as out: + subprocess.run([test_path], stdout=out, stderr=subprocess.STDOUT, check=False) + else: + logging.warning("Test binary not found at %s", test_path) - # Include doctest results in the log file - with open(output_file, "r") as f: - doctest_results = f.read() - logging.info("Doctest results:\n{}".format(doctest_results)) + # Read and log results + if os.path.exists(output_file): + with open(output_file, 'r', encoding='utf-8') as f: + doctest_results = f.read() + logging.info("Test output for %s:\n%s", cfg_name, doctest_results) + else: + logging.warning("No test output file produced for %s", cfg_name) - # Clean up - os.remove(output_file) - logging.info("Removed doctest output file.") + except subprocess.CalledProcessError as e: + logging.error("Build or command failed for %s: %s", cfg_name, str(e)) + except Exception as e: + logging.exception("Unexpected error in configuration %s: %s", cfg_name, str(e)) + finally: + # Optionally keep outputs or clean up + logging.info("Completed configuration: %s", cfg_name) - # Print status message - print("Done.") - logging.info("Script finished.") \ No newline at end of file +print("All configurations finished.") \ No newline at end of file diff --git a/src/configuration_module.cpp b/src/configuration_module.cpp index e8f70d04..4db1e5b9 100644 --- a/src/configuration_module.cpp +++ b/src/configuration_module.cpp @@ -382,7 +382,7 @@ class ConfigurationModule::ConfigurationModuleImpl { } void _stopTimer() { - _logger->log(LogLevel::WARNING, "[Countly] [ConfigurationModule] stopTimer, stopping server config update timer thread."); + _logger->log(LogLevel::INFO, "[Countly] [ConfigurationModule] stopTimer, stopping server config update timer thread."); stopConfigThread.store(true, std::memory_order_release); configUpdateCv.notify_all(); diff --git a/src/countly.cpp b/src/countly.cpp index d5e2a6f4..5c5d67ad 100644 --- a/src/countly.cpp +++ b/src/countly.cpp @@ -1,13 +1,16 @@ #include "countly/internal_limits.hpp" +#include "countly/path_utils.hpp" #include "countly/storage_module_db.hpp" #include "countly/storage_module_memory.hpp" #include #include #include +#include #include #include #include #include +#include #ifndef COUNTLY_USE_CUSTOM_SHA256 #include "openssl/sha.h" @@ -20,7 +23,18 @@ #endif namespace cly { + +namespace { +/** + * Number of live Countly objects in this process, registry-held and + * integrator-owned alike. shutdownNetworking() refuses to tear down networking + * while this is non-zero. + */ +std::atomic live_instance_count(0); +} // namespace + Countly::Countly() { + live_instance_count.fetch_add(1); crash_module = nullptr; views_module = nullptr; logger.reset(new cly::LoggerModule()); @@ -30,28 +44,217 @@ Countly::Countly() { Countly::~Countly() { is_being_disposed = true; stop(); + releaseDatabasePathClaim(); crash_module.reset(); views_module.reset(); configurationModule.reset(); logger.reset(); + live_instance_count.fetch_sub(1); +} + +namespace { + +const char *const DEFAULT_INSTANCE_NAME = ""; + +/** + * Process-wide state shared by every Countly instance: + * - the named-instance table, including the unnamed default instance + * - the set of claimed SQLite database paths + * + * Reached only through registry(), so initialisation is thread-safe by way of + * C++11 function-local statics. + * + * The constructor calls RequestModule::initGlobalNetworking() so libcurl's + * global guard finishes construction *before* this registry and is therefore + * destroyed *after* it. That matters at process exit: the registry destroys its + * remaining instances, and ~Countly still needs networking for endSession(). + */ +class InstanceRegistry { +public: + InstanceRegistry() { RequestModule::initGlobalNetworking(); } + + ~InstanceRegistry() { clear(); } + + /** + * Returns the instance registered under 'name', creating it if absent. + * *created_out reports whether this call created it. + */ + std::shared_ptr getOrCreate(const std::string &name, bool *created_out) { + std::shared_ptr fresh; + { + std::lock_guard lk(_instances_mutex); + for (size_t index = 0; index < _instances.size(); index++) { + if (_instances[index].first == name) { + if (created_out != nullptr) { + *created_out = false; + } + return _instances[index].second; + } + } + fresh.reset(new Countly()); + _instances.push_back(std::make_pair(name, fresh)); + } + if (created_out != nullptr) { + *created_out = true; + } + return fresh; + } + + std::shared_ptr find(const std::string &name) { + std::lock_guard lk(_instances_mutex); + for (size_t index = 0; index < _instances.size(); index++) { + if (_instances[index].first == name) { + return _instances[index].second; + } + } + return std::shared_ptr(); + } + + /** + * Unregisters 'name' and hands its instance back so the caller can let it drop + * *outside* the lock. ~Countly calls releasePath(), so destroying while + * _instances_mutex is held would be a latent self-deadlock. + */ + std::shared_ptr detach(const std::string &name) { + std::lock_guard lk(_instances_mutex); + for (std::vector>>::iterator it = _instances.begin(); it != _instances.end(); ++it) { + if (it->first == name) { + std::shared_ptr found = it->second; + _instances.erase(it); + return found; + } + } + return std::shared_ptr(); + } + + /** Destroys every instance in reverse creation order, outside the lock. */ + void clear() { + std::vector> doomed; + { + std::lock_guard lk(_instances_mutex); + for (size_t index = _instances.size(); index > 0; index--) { + doomed.push_back(_instances[index - 1].second); + } + _instances.clear(); + } + for (size_t index = 0; index < doomed.size(); index++) { + doomed[index].reset(); // ~Countly runs here, with no registry lock held + } + } + + /** @return false if the path is already claimed by a live instance. */ + bool tryClaimPath(const std::string &normalized) { + std::lock_guard lk(_paths_mutex); + return _claimed_paths.insert(normalized).second; + } + + void releasePath(const std::string &normalized) { + std::lock_guard lk(_paths_mutex); + _claimed_paths.erase(normalized); + } + + size_t claimedPathCount() { + std::lock_guard lk(_paths_mutex); + return _claimed_paths.size(); + } + +private: + // Two mutexes, never nested in the other direction: _paths_mutex is always the + // innermost lock, because start() claims a path while holding the instance's + // own mutex. + std::mutex _instances_mutex; + std::vector>> _instances; + std::mutex _paths_mutex; + std::set _claimed_paths; +}; + +InstanceRegistry ®istry() { + static InstanceRegistry instance; + return instance; +} + +} // namespace + +void Countly::inheritDefaultLogger(Countly &instance) { + std::shared_ptr defaultInstance = registry().find(DEFAULT_INSTANCE_NAME); + if (!defaultInstance || defaultInstance.get() == &instance) { + return; + } + LoggerFunction callback = defaultInstance->logger->getLogger(); + if (callback != nullptr) { + instance.logger->setLogger(callback); + } +} + +void Countly::releaseDatabasePathClaim() { +#ifdef COUNTLY_USE_SQLITE + if (!claimed_database_path.empty()) { + registry().releasePath(claimed_database_path); + claimed_database_path.clear(); + } +#endif +} + +Countly &Countly::getInstance() { return getInstance(DEFAULT_INSTANCE_NAME); } + +Countly &Countly::getInstance(const std::string &name) { + bool created = false; + std::shared_ptr instance = registry().getOrCreate(name, &created); + if (created && !name.empty()) { + inheritDefaultLogger(*instance); + instance->log(LogLevel::WARNING, "[Countly] getInstance, creating a new, uninitialized instance named '" + name + "'; if you expected an existing instance, check the name."); + } + return *instance; } -std::unique_ptr _sharedInstance; -Countly &Countly::getInstance() { - if (_sharedInstance.get() == nullptr) { - _sharedInstance.reset(new Countly()); +Countly &Countly::createInstance(const std::string &name) { + bool created = false; + std::shared_ptr instance = registry().getOrCreate(name, &created); + if (created) { + inheritDefaultLogger(*instance); + } else { + instance->log(LogLevel::WARNING, "[Countly] createInstance, an instance named '" + name + "' already exists; returning the existing one."); } + return *instance; +} + +Countly *Countly::findInstance(const std::string &name) { + std::shared_ptr instance = registry().find(name); + return instance ? instance.get() : nullptr; +} + +bool Countly::hasInstance(const std::string &name) { return findInstance(name) != nullptr; } - return *_sharedInstance.get(); +void Countly::destroyInstance(const std::string &name) { + std::shared_ptr doomed = registry().detach(name); + doomed.reset(); // ~Countly runs here, with no registry lock held +} + +void Countly::destroyAllInstances() { registry().clear(); } + +void Countly::shutdownNetworking() { + const int live = live_instance_count.load(); + if (live > 0) { + // Log through the default instance if one exists -- a diagnostic must not + // create an instance as a side effect. + std::shared_ptr defaultInstance = registry().find(DEFAULT_INSTANCE_NAME); + if (defaultInstance) { + defaultInstance->log(LogLevel::ERROR, "[Countly] shutdownNetworking, " + std::to_string(live) + " instance(s) are still live; refusing to tear down networking."); + } + return; + } + RequestModule::releaseGlobalNetworking(); } #ifdef COUNTLY_BUILD_TESTS void Countly::halt() { - if (_sharedInstance) { - _sharedInstance->stop(); - } - _sharedInstance.reset(new Countly()); + destroyAllInstances(); + getInstance(); // leave a fresh default instance behind, as the old halt() did } + +size_t Countly::debugClaimedPathCount() { return registry().claimedPathCount(); } + +int Countly::debugLiveInstanceCount() { return live_instance_count.load(); } #endif /** @@ -519,6 +722,17 @@ void Countly::start(const std::string &app_key, const std::string &host, int por log(LogLevel::ERROR, "[Countly] start, Database path can not be empty or blank."); return; } + + // One instance per database file. Two instances sharing one file would eat + // each other's queues, and because rows in the `events` table carry no + // app_key, one instance would pack the other's events into a request stamped + // with the wrong key. + const std::string normalized_database_path = cly::utils::normalizeDatabasePath(configuration->databasePath); + if (!registry().tryClaimPath(normalized_database_path)) { + log(LogLevel::ERROR, "[Countly] start, Database path '" + configuration->databasePath + "' is already in use by another Countly instance in this process. SDK will not be initialized."); + return; + } + claimed_database_path = normalized_database_path; #endif log(LogLevel::INFO, "[Countly] start, Initializing SDK"); @@ -589,6 +803,7 @@ void Countly::start(const std::string &app_key, const std::string &host, int por is_sdk_initialized = result; // after this point SDK is initialized. if (!is_sdk_initialized) { log(LogLevel::ERROR, "[Countly] start, SDK initialization failed."); + releaseDatabasePathClaim(); return; } @@ -630,7 +845,28 @@ void Countly::startOnCloud(const std::string &app_key) { this->start(app_key, "https://cloud.count.ly", 443); } +void Countly::joinRemoteConfigThreads() { + std::lock_guard lk(remote_config_thread_mutex); + for (size_t index = 0; index < remote_config_threads.size(); index++) { + if (remote_config_threads[index].joinable()) { + try { + remote_config_threads[index].join(); + } catch (const std::system_error &e) { + log(LogLevel::WARNING, std::string("[Countly] joinRemoteConfigThreads, Could not join thread: ") + e.what()); + } + } + } + remote_config_threads.clear(); +} + void Countly::stop() { + // configurationModule is only constructed inside start(), so it is null on an + // instance that was never started -- createInstance() followed by + // destroyInstance(), or a start() that was refused. Guard every call into it. + if (configurationModule) { + configurationModule->stopTimer(); + } + joinRemoteConfigThreads(); _deleteThread(); if (configuration->manualSessionControl == false) { endSession(); @@ -1582,9 +1818,14 @@ void Countly::updateRemoteConfig() { lk.unlock(); - // Fetch remote config asynchronously - std::thread _thread(&Countly::_fetchRemoteConfig, this, data); - _thread.detach(); + // Fetch remote config asynchronously on an owned thread. Any previous fetch is + // joined first, so back-to-back calls serialise rather than piling up threads + // that capture `this`. + joinRemoteConfigThreads(); + { + std::lock_guard tlk(remote_config_thread_mutex); + remote_config_threads.push_back(std::thread(&Countly::_fetchRemoteConfig, this, data)); + } } nlohmann::json Countly::getRemoteConfigValue(const std::string &key) { @@ -1625,9 +1866,14 @@ void Countly::updateRemoteConfigFor(std::string *keys, size_t key_count) { } lk.unlock(); - // Fetch remote config asynchronously - std::thread _thread(&Countly::_updateRemoteConfigWithSpecificValues, this, data); - _thread.detach(); + // Fetch remote config asynchronously on an owned thread. Any previous fetch is + // joined first, so back-to-back calls serialise rather than piling up threads + // that capture `this`. + joinRemoteConfigThreads(); + { + std::lock_guard tlk(remote_config_thread_mutex); + remote_config_threads.push_back(std::thread(&Countly::_updateRemoteConfigWithSpecificValues, this, data)); + } } void Countly::updateRemoteConfigExcept(std::string *keys, size_t key_count) { @@ -1647,8 +1893,13 @@ void Countly::updateRemoteConfigExcept(std::string *keys, size_t key_count) { } lk.unlock(); - // Fetch remote config asynchronously - std::thread _thread(&Countly::_updateRemoteConfigWithSpecificValues, this, data); - _thread.detach(); + // Fetch remote config asynchronously on an owned thread. Any previous fetch is + // joined first, so back-to-back calls serialise rather than piling up threads + // that capture `this`. + joinRemoteConfigThreads(); + { + std::lock_guard tlk(remote_config_thread_mutex); + remote_config_threads.push_back(std::thread(&Countly::_updateRemoteConfigWithSpecificValues, this, data)); + } } } // namespace cly diff --git a/src/request_module.cpp b/src/request_module.cpp index d6983d06..ed8aca24 100644 --- a/src/request_module.cpp +++ b/src/request_module.cpp @@ -1,6 +1,7 @@ #include "countly/request_module.hpp" #include "countly/request_builder.hpp" +#include #include #include #include @@ -20,6 +21,12 @@ #endif #endif +#if !defined(_WIN32) && !defined(COUNTLY_USE_CUSTOM_HTTP) +#define COUNTLY_HAS_CURL 1 +#else +#define COUNTLY_HAS_CURL 0 +#endif + namespace cly { class RequestModule::RequestModuleImpl { private: @@ -75,23 +82,74 @@ class RequestModule::RequestModuleImpl { } }; -RequestModule::RequestModule(std::shared_ptr config, std::shared_ptr logger, std::shared_ptr requestBuilder, std::shared_ptr storageModule) { - impl.reset(new RequestModuleImpl(config, logger, requestBuilder, storageModule)); +namespace { +/** + * Process-wide libcurl initialisation. + * + * libcurl's global init/cleanup pair is process-wide, and curl_global_cleanup() + * must not run while another thread is using curl. Calling it when one SDK + * instance is destroyed would tear networking down under every other live + * instance, so cleanup happens exactly once: either at process exit (this is a + * function-local static) or when the integrator calls + * Countly::shutdownNetworking(). + * + * The counters are maintained even on builds without curl so the tests mean the + * same thing on every platform. + */ +class CurlGlobal { +public: + CurlGlobal() { +#if COUNTLY_HAS_CURL + curl_global_init(CURL_GLOBAL_ALL); +#endif + _init_count.fetch_add(1); + } - impl->_logger->log(LogLevel::DEBUG, cly::utils::format_string("[Countly] [RequestModule] Initialized")); + ~CurlGlobal() { release(); } -#if !defined(_WIN32) && !defined(COUNTLY_USE_CUSTOM_HTTP) - curl_global_init(CURL_GLOBAL_ALL); + void release() { + if (_released.exchange(true)) { + return; + } +#if COUNTLY_HAS_CURL + curl_global_cleanup(); #endif + } + + int initCount() const { return _init_count.load(); } + bool released() const { return _released.load(); } + +private: + std::atomic _init_count{0}; + std::atomic _released{false}; +}; + +CurlGlobal &curlGlobal() { + static CurlGlobal instance; + return instance; } +} // namespace -RequestModule::~RequestModule() { - impl.reset(); -#if !defined(_WIN32) && !defined(COUNTLY_USE_CUSTOM_HTTP) - curl_global_cleanup(); +void RequestModule::initGlobalNetworking() { curlGlobal(); } + +void RequestModule::releaseGlobalNetworking() { curlGlobal().release(); } + +#ifdef COUNTLY_BUILD_TESTS +int RequestModule::globalNetworkingInitCount() { return curlGlobal().initCount(); } + +bool RequestModule::globalNetworkingReleased() { return curlGlobal().released(); } #endif + +RequestModule::RequestModule(std::shared_ptr config, std::shared_ptr logger, std::shared_ptr requestBuilder, std::shared_ptr storageModule) { + impl.reset(new RequestModuleImpl(config, logger, requestBuilder, storageModule)); + + impl->_logger->log(LogLevel::DEBUG, cly::utils::format_string("[Countly] [RequestModule] Initialized")); + + initGlobalNetworking(); } +RequestModule::~RequestModule() { impl.reset(); } + static size_t countly_curl_write_callback(void *data, size_t byte_size, size_t n_bytes, std::string *body) { size_t data_size = byte_size * n_bytes; body->append((const char *)data, data_size); diff --git a/tests/multi_instance.cpp b/tests/multi_instance.cpp new file mode 100644 index 00000000..a547d5ce --- /dev/null +++ b/tests/multi_instance.cpp @@ -0,0 +1,402 @@ +#include +#include +#include +#include +#include + +#include "doctest.h" + +#include "countly/constants.hpp" +#include "countly/path_utils.hpp" +#include "countly/request_module.hpp" +#include "test_utils.hpp" + +using namespace cly; +using namespace test_utils; + +// Countly::setLogger takes a raw function pointer, not a std::function, so a +// capturing lambda will not convert. +static void multiInstanceTestLogger(cly::LogLevel level, const std::string &message) { + (void)level; + (void)message; +} + +TEST_CASE("normalizeDatabasePath collapses equivalent spellings") { + // All inputs are lowercase so the expectations hold on both case-sensitive + // and case-insensitive platforms. Case folding is covered separately below. + CHECK(utils::normalizeDatabasePath("a.db") == "a.db"); + CHECK(utils::normalizeDatabasePath("./a.db") == "a.db"); + CHECK(utils::normalizeDatabasePath(".\\a.db") == "a.db"); + CHECK(utils::normalizeDatabasePath("data//a.db") == "data/a.db"); + CHECK(utils::normalizeDatabasePath("data/./a.db") == "data/a.db"); + CHECK(utils::normalizeDatabasePath("data/../data/a.db") == "data/a.db"); + CHECK(utils::normalizeDatabasePath("data\\sub\\..\\a.db") == "data/a.db"); + CHECK(utils::normalizeDatabasePath(" a.db ") == "a.db"); + CHECK(utils::normalizeDatabasePath("data/") == "data"); + CHECK(utils::normalizeDatabasePath("/var/db/a.db") == "/var/db/a.db"); + CHECK(utils::normalizeDatabasePath("//server/share/a.db") == "//server/share/a.db"); + CHECK(utils::normalizeDatabasePath("../a.db") == "../a.db"); + CHECK(utils::normalizeDatabasePath("c:/data/a.db") == "c:/data/a.db"); + CHECK(utils::normalizeDatabasePath("") == ""); + CHECK(utils::normalizeDatabasePath(" ") == ""); +} + +#ifdef _WIN32 +TEST_CASE("normalizeDatabasePath folds case on Windows") { + CHECK(utils::normalizeDatabasePath("A.DB") == "a.db"); + CHECK(utils::normalizeDatabasePath("C:\\Data\\A.db") == "c:/data/a.db"); +} +#endif + +TEST_CASE("generateEventID varies its random component") { + // The old implementation bound a *copy* of a const engine on every call, so + // the random half was constant for the whole process and only the timestamp + // moved. Assert on the random half directly: a uniqueness check on the whole + // ID can pass by accident on platforms with a fine-grained clock. + std::set random_parts; + std::set ids; + for (int i = 0; i < 1000; i++) { + const std::string id = utils::generateEventID(); + ids.insert(id); + random_parts.insert(id.substr(0, id.find('_'))); + } + CHECK(random_parts.size() > 1); + CHECK(ids.size() == 1000); +} + +TEST_CASE("two instances produce distinct view IDs for the same view name") { + clearSDK(); + InstanceFixture a("APP_KEY_A", "mi-viewid-a.db"); + InstanceFixture b("APP_KEY_B", "mi-viewid-b.db"); + REQUIRE(a.initialized()); + REQUIRE(b.initialized()); + + const std::string idA = a.sdk->views().openView("home"); + const std::string idB = b.sdk->views().openView("home"); + + CHECK(idA != ""); + CHECK(idB != ""); + CHECK(idA != idB); +} + +TEST_CASE("events recorded on one instance never reach the other") { + clearSDK(); + InstanceFixture a("APP_KEY_A", "mi-events-a.db"); + InstanceFixture b("APP_KEY_B", "mi-events-b.db"); + REQUIRE(a.initialized()); + REQUIRE(b.initialized()); + a.clearCalls(); + b.clearCalls(); + + a.sdk->addEvent(cly::Event("click", 1)); + b.sdk->addEvent(cly::Event("purchase", 1)); + a.sdk->flushEvents(); + b.sdk->flushEvents(); + a.flush(); + b.flush(); + + CHECK(a.sawEvent("click")); + CHECK_FALSE(a.sawEvent("purchase")); + CHECK(b.sawEvent("purchase")); + CHECK_FALSE(b.sawEvent("click")); + CHECK(a.sawKeyValue("app_key", "APP_KEY_A")); + CHECK_FALSE(a.sawKeyValue("app_key", "APP_KEY_B")); + CHECK(b.sawKeyValue("app_key", "APP_KEY_B")); + CHECK_FALSE(b.sawKeyValue("app_key", "APP_KEY_A")); +} + +TEST_CASE("each instance's session carries its own app key and device id") { + clearSDK(); + InstanceFixture a("APP_KEY_A", "mi-session-a.db", "device-a"); + InstanceFixture b("APP_KEY_B", "mi-session-b.db", "device-b"); + REQUIRE(a.initialized()); + REQUIRE(b.initialized()); + a.flush(); + b.flush(); + + CHECK(a.sawKeyValue("device_id", "device-a")); + CHECK_FALSE(a.sawKeyValue("device_id", "device-b")); + CHECK(b.sawKeyValue("device_id", "device-b")); + CHECK_FALSE(b.sawKeyValue("device_id", "device-a")); +} + +TEST_CASE("destroying one instance leaves the other working") { + clearSDK(); + InstanceFixture b("APP_KEY_B", "mi-survive-b.db"); + REQUIRE(b.initialized()); + { + InstanceFixture a("APP_KEY_A", "mi-survive-a.db"); + REQUIRE(a.initialized()); + a.sdk->addEvent(cly::Event("click", 1)); + } + // a is gone; b must still record and deliver. + b.clearCalls(); + b.sdk->addEvent(cly::Event("purchase", 1)); + b.sdk->flushEvents(); + b.flush(); + + CHECK(b.initialized()); + CHECK(b.sawEvent("purchase")); +} + +TEST_CASE("curl global state is initialised once and survives instance destruction") { + clearSDK(); + InstanceFixture a("APP_KEY_A", "mi-curl-a.db"); + REQUIRE(a.initialized()); + CHECK(cly::RequestModule::globalNetworkingInitCount() == 1); + + { + InstanceFixture b("APP_KEY_B", "mi-curl-b.db"); + REQUIRE(b.initialized()); + CHECK(cly::RequestModule::globalNetworkingInitCount() == 1); + } + + // b is destroyed. Networking must not have been torn down under a. + CHECK(cly::RequestModule::globalNetworkingReleased() == false); + CHECK(a.initialized()); + CHECK(cly::RequestModule::globalNetworkingInitCount() == 1); +} + +TEST_CASE("named instances are distinct objects and are found by name") { + clearSDK(); + cly::Countly &a = cly::Countly::createInstance("appA"); + cly::Countly &b = cly::Countly::createInstance("appB"); + + CHECK(&a != &b); + CHECK(&cly::Countly::getInstance("appA") == &a); + CHECK(&cly::Countly::getInstance("appB") == &b); + CHECK(&cly::Countly::getInstance() != &a); + CHECK(&cly::Countly::getInstance() != &b); + + CHECK(cly::Countly::hasInstance("appA")); + CHECK(cly::Countly::findInstance("appA") == &a); +} + +TEST_CASE("the empty name is the default instance") { + clearSDK(); + CHECK(&cly::Countly::getInstance("") == &cly::Countly::getInstance()); +} + +TEST_CASE("an unknown name is not reported as present") { + clearSDK(); + CHECK_FALSE(cly::Countly::hasInstance("nope")); + CHECK(cly::Countly::findInstance("nope") == nullptr); +} + +TEST_CASE("destroyInstance removes the instance") { + clearSDK(); + cly::Countly::createInstance("appA"); + REQUIRE(cly::Countly::hasInstance("appA")); + + cly::Countly::destroyInstance("appA"); + + CHECK_FALSE(cly::Countly::hasInstance("appA")); + CHECK(cly::Countly::findInstance("appA") == nullptr); + cly::Countly::destroyInstance("appA"); // destroying an absent name is a no-op + CHECK_FALSE(cly::Countly::hasInstance("appA")); +} + +TEST_CASE("destroyAllInstances clears the registry") { + clearSDK(); + cly::Countly::createInstance("appA"); + cly::Countly::createInstance("appB"); + + cly::Countly::destroyAllInstances(); + + CHECK_FALSE(cly::Countly::hasInstance("appA")); + CHECK_FALSE(cly::Countly::hasInstance("appB")); +} + +TEST_CASE("a registry-created instance inherits the default instance's logger") { + clearSDK(); + cly::Countly::getInstance().setLogger(multiInstanceTestLogger); + REQUIRE(cly::Countly::getInstance().getLogger() != nullptr); + + cly::Countly &named = cly::Countly::createInstance("appLogger"); + + CHECK(named.getLogger() != nullptr); +} + +TEST_CASE("shutdownNetworking refuses while an instance is live") { + clearSDK(); + InstanceFixture a("APP_KEY_A", "mi-shutdown-a.db"); + REQUIRE(a.initialized()); + REQUIRE(cly::Countly::debugLiveInstanceCount() > 0); + + cly::Countly::shutdownNetworking(); + + CHECK(cly::RequestModule::globalNetworkingReleased() == false); + CHECK(a.initialized()); +} + +TEST_CASE("a remote config fetch thread is joined by destruction") { + clearSDK(); + static std::atomic fetch_completed(false); + fetch_completed.store(false); + + std::shared_ptr sdk = std::make_shared(); + // A slow client: if the fetch thread is detached, destruction returns before + // the store below runs, which is exactly the bug this asserts against. + sdk->setHTTPClient([](bool use_post, const std::string &url, const std::string &data) { + (void)use_post; + (void)url; + cly::HTTPResponse response; + response.success = true; + response.data = nlohmann::json::object(); + // Only the remote-config fetch is slowed down. The SDK Behavior Settings + // fetch also targets /o/sdk (with method=sc), and delaying that one too + // would let the SBS thread set the flag and make this test meaningless. + if (data.find("fetch_remote_config") != std::string::npos) { + std::this_thread::sleep_for(std::chrono::milliseconds(250)); + fetch_completed.store(true); + } + return response; + }); + sdk->setDeviceID(COUNTLY_TEST_DEVICE_ID); + sdk->SetPath("mi-remote-config.db"); + sdk->disableSDKBehaviorSettingsUpdates(); // no periodic SBS thread in this test + // Without this, ~Countly blocks for up to COUNTLY_KEEPALIVE_INTERVAL (3s) + // joining the update loop, and that incidental wait is long enough for a + // *detached* fetch thread to finish -- which would make this test pass + // whether the thread is owned or not. With it, destruction returns promptly, + // so only an owned-and-joined thread can have set the flag. + sdk->enableImmediateRequestOnStop(); + sdk->enableRemoteConfig(); + sdk->start("APP_KEY_RC", COUNTLY_TEST_HOST, COUNTLY_TEST_PORT, false); + REQUIRE(sdk->checkEQSize() == 0); + + fetch_completed.store(false); + sdk->updateRemoteConfig(); + sdk.reset(); // must join the fetch thread + + CHECK(fetch_completed.load() == true); + remove("mi-remote-config.db"); +} + +#ifdef COUNTLY_USE_SQLITE + +// Builds a second instance by hand, because InstanceFixture assumes start() +// succeeds and here we need to observe it being refused. +static std::shared_ptr startInstanceAt(const std::string &app_key, const std::string &db_path) { + std::shared_ptr sdk = std::make_shared(); + sdk->setHTTPClient(test_utils::fakeSendHTTP); + sdk->setDeviceID(COUNTLY_TEST_DEVICE_ID); + sdk->SetPath(db_path); + sdk->start(app_key, COUNTLY_TEST_HOST, COUNTLY_TEST_PORT, false); + return sdk; +} + +TEST_CASE("a second instance on a claimed database path is refused") { + clearSDK(); + InstanceFixture a("APP_KEY_A", "mi-claim.db"); + REQUIRE(a.initialized()); + + std::shared_ptr b = startInstanceAt("APP_KEY_B", "mi-claim.db"); + + CHECK(b->checkEQSize() == -1); // refused: never initialized + CHECK(a.initialized()); // the first instance is unaffected + b.reset(); +} + +TEST_CASE("an equivalent spelling of a claimed path is refused") { + clearSDK(); + InstanceFixture a("APP_KEY_A", "mi-equiv.db"); + REQUIRE(a.initialized()); + + std::shared_ptr b = startInstanceAt("APP_KEY_B", "./mi-equiv.db"); + + CHECK(b->checkEQSize() == -1); + CHECK(a.initialized()); + b.reset(); +} + +TEST_CASE("a database path is released when its instance is destroyed") { + clearSDK(); + { + InstanceFixture a("APP_KEY_A", "mi-release.db"); + REQUIRE(a.initialized()); + } + InstanceFixture b("APP_KEY_B", "mi-release.db"); + CHECK(b.initialized()); +} + +TEST_CASE("halt releases the default instance's path claim") { + clearSDK(); + cly::Countly &d = cly::Countly::getInstance(); + d.setHTTPClient(test_utils::fakeSendHTTP); + d.setDeviceID(COUNTLY_TEST_DEVICE_ID); + d.SetPath(TEST_DATABASE_NAME); + d.start(COUNTLY_TEST_APP_KEY, COUNTLY_TEST_HOST, COUNTLY_TEST_PORT, false); + REQUIRE(d.checkEQSize() == 0); + + clearSDK(); // halt() -> destroyAllInstances() + + CHECK(cly::Countly::debugClaimedPathCount() == 0); +} + +TEST_CASE("a failed initialisation leaves no stale path claim") { + clearSDK(); + const size_t before = cly::Countly::debugClaimedPathCount(); + + // Blank path: refused before any claim is made. + std::shared_ptr blank = startInstanceAt("APP_KEY_A", ""); + CHECK(blank->checkEQSize() == -1); + CHECK(cly::Countly::debugClaimedPathCount() == before); + blank.reset(); + + // Unwritable path: the claim is taken, then the schema creation fails, so the + // claim must be given back. + std::shared_ptr bad = startInstanceAt("APP_KEY_A", "mi-no-such-dir/x.db"); + CHECK(bad->checkEQSize() == -1); + CHECK(cly::Countly::debugClaimedPathCount() == before); + bad.reset(); +} + +TEST_CASE("destroyInstance releases the named instance's path claim") { + clearSDK(); + const size_t before = cly::Countly::debugClaimedPathCount(); + cly::Countly &named = cly::Countly::createInstance("appPath"); + named.setHTTPClient(test_utils::fakeSendHTTP); + named.setDeviceID(COUNTLY_TEST_DEVICE_ID); + named.SetPath("mi-named.db"); + named.start("APP_KEY_NAMED", COUNTLY_TEST_HOST, COUNTLY_TEST_PORT, false); + REQUIRE(named.checkEQSize() == 0); + REQUIRE(cly::Countly::debugClaimedPathCount() == before + 1); + + cly::Countly::destroyInstance("appPath"); + + CHECK(cly::Countly::debugClaimedPathCount() == before); + CHECK_FALSE(cly::Countly::hasInstance("appPath")); + remove("mi-named.db"); +} + +TEST_CASE("start can be retried once a refused path is free") { + clearSDK(); + std::shared_ptr b; + { + InstanceFixture a("APP_KEY_A", "mi-retry.db"); + REQUIRE(a.initialized()); + + b = startInstanceAt("APP_KEY_B", "mi-retry.db"); + CHECK(b->checkEQSize() == -1); + } + // a is destroyed, so the path is free again. + b->start("APP_KEY_B", COUNTLY_TEST_HOST, COUNTLY_TEST_PORT, false); + CHECK(b->checkEQSize() == 0); + b.reset(); + remove("mi-retry.db"); +} + +#endif // COUNTLY_USE_SQLITE + +// NOTE: keep this the LAST test case in this file. Releasing libcurl's global +// state cannot be undone for the process. Every other test drives HTTP through +// the fake client, so a released curl does not affect them. +TEST_CASE("shutdownNetworking releases once no instance is live") { + cly::Countly::destroyAllInstances(); + REQUIRE(cly::Countly::debugLiveInstanceCount() == 0); + + cly::Countly::shutdownNetworking(); + + CHECK(cly::RequestModule::globalNetworkingReleased() == true); +} diff --git a/tests/test_utils.hpp b/tests/test_utils.hpp index 06638104..9b136d32 100644 --- a/tests/test_utils.hpp +++ b/tests/test_utils.hpp @@ -110,7 +110,7 @@ static long long getUnixTimestamp() { return timestamp.count(); } -static HTTPResponse fakeSendHTTP(bool use_post, const std::string &url, const std::string &data) { +static HTTPResponse fakeSendHTTPInto(ThreadSafeHTTPCallQueue &queue, bool use_post, const std::string &url, const std::string &data) { HTTPCall http_call({use_post, url, {}}); std::string::size_type startIndex = 0; @@ -135,7 +135,7 @@ static HTTPResponse fakeSendHTTP(bool use_post, const std::string &url, const st } } - http_call_queue.push_back(http_call); + queue.push_back(http_call); HTTPResponse response{false, nlohmann::json::object()}; @@ -170,6 +170,10 @@ static HTTPResponse fakeSendHTTP(bool use_post, const std::string &url, const st return response; } +// Every existing test drives HTTP through the one global queue; multi-instance +// tests pass their own queue instead so each instance can be inspected alone. +static HTTPResponse fakeSendHTTP(bool use_post, const std::string &url, const std::string &data) { return fakeSendHTTPInto(http_call_queue, use_post, url, data); } + // Search http_call_queue for a request containing a specific key=value pair static bool httpQueueContains(const std::string &key, const std::string &value) { size_t n = http_call_queue.size(); @@ -227,6 +231,90 @@ static void initCountlyWithFakeNetworking(bool clearInitialNetworkingState, cly: http_call_queue.clear(); // cl+ear local HTTP request queue. } } + +/** + * Owns one Countly instance together with its own HTTP capture queue and its + * own database file, so a test can assert that two instances never see each + * other's data. + * + * Not copyable or movable: the HTTP client lambda captures `this`. + */ +struct InstanceFixture { + ThreadSafeHTTPCallQueue calls; + std::shared_ptr sdk; + std::string appKey; + std::string dbPath; + + InstanceFixture(const std::string &app_key, const std::string &db_path, const std::string &device_id = COUNTLY_TEST_DEVICE_ID) : appKey(app_key), dbPath(db_path) { + remove(dbPath.c_str()); + sdk = std::make_shared(); + sdk->setHTTPClient([this](bool use_post, const std::string &url, const std::string &data) { return fakeSendHTTPInto(calls, use_post, url, data); }); + sdk->setDeviceID(device_id); + sdk->SetPath(dbPath); + sdk->start(appKey, COUNTLY_TEST_HOST, COUNTLY_TEST_PORT, false); + // The SBS config fetch runs on its own thread; give it time to land so it + // does not interleave with the assertions. + std::this_thread::sleep_for(std::chrono::milliseconds(200)); + } + + ~InstanceFixture() { + sdk.reset(); + remove(dbPath.c_str()); + } + + InstanceFixture(const InstanceFixture &) = delete; + InstanceFixture &operator=(const InstanceFixture &) = delete; + + /** + * checkEQSize() returns -1 before the SDK is initialized in both the SQLite + * and the in-memory build (src/countly.cpp:1196-1206), which makes it a + * reliable proxy for "did start() succeed". + */ + bool initialized() const { return sdk && sdk->checkEQSize() >= 0; } + + /** Drains the request queue into `calls`. */ + void flush() { sdk->processRQDebug(); } + + /** Drops the request queue and everything captured so far. */ + void clearCalls() { + sdk->clearRequestQueue(); + calls.clear(); + } + + bool sawKeyValue(const std::string &key, const std::string &value) { + const size_t count = calls.size(); + for (size_t index = 0; index < count; index++) { + HTTPCall call = calls.at(index); + std::map::const_iterator found = call.data.find(key); + if (found != call.data.end() && found->second == value) { + return true; + } + } + return false; + } + + bool sawEvent(const std::string &event_key) { + const size_t count = calls.size(); + for (size_t index = 0; index < count; index++) { + HTTPCall call = calls.at(index); + std::map::const_iterator found = call.data.find("events"); + if (found == call.data.end()) { + continue; + } + try { + nlohmann::json events = nlohmann::json::parse(found->second); + for (nlohmann::json::const_iterator event = events.begin(); event != events.end(); ++event) { + if ((*event)["key"].get() == event_key) { + return true; + } + } + } catch (const nlohmann::json::exception &) { + // Malformed events JSON -- skip this entry + } + } + return false; + } +}; } // namespace test_utils #endif From 9c46c674a326093950a6352abf024a3e171e0c3a Mon Sep 17 00:00:00 2001 From: turtledreams <62231246+turtledreams@users.noreply.github.com> Date: Wed, 5 Aug 2026 20:11:34 +0900 Subject: [PATCH 2/5] fixes --- CHANGELOG.md | 4 +- include/countly.hpp | 29 +++- include/countly/request_module.hpp | 25 +-- src/configuration_module.cpp | 13 +- src/countly.cpp | 238 +++++++++++++++++++---------- src/logger_module.cpp | 38 ++++- src/request_module.cpp | 15 +- tests/multi_instance.cpp | 56 ++++++- 8 files changed, 315 insertions(+), 103 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d34bbb84..0c8c5c5c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,8 +5,8 @@ - ! Minor breaking change ! When built with SQLite, each instance requires its own database path. A second instance claiming a path already in use logs an error and does not initialize. - Fixed the libcurl global lifecycle: `curl_global_init` now runs once per process, and cleanup no longer runs when an instance is destroyed, which could tear down networking underneath another live instance. Added `shutdownNetworking()` for hosts that load and unload the SDK without exiting. - Fixed non-unique event and view IDs: the random component of generated IDs was constant for the lifetime of the process, and on platforms with a coarse `system_clock` (Windows, ~15ms) the timestamp component did not change either, so IDs generated within one tick were identical. -- Remote config fetches now run on owned threads that are joined by `stop()` and by destruction, instead of being detached. Consecutive remote config calls block until the previous fetch completes. -- `stop()` now also stops periodic SDK Behavior Settings updates. +- Remote config fetches now run on an owned thread that is joined when the SDK is destroyed, instead of being detached, which could leave a fetch running after the objects it used were gone. Only one fetch runs at a time per instance: a call made while a fetch is in flight is logged and ignored rather than queued, so `updateRemoteConfig`, `updateRemoteConfigFor` and `updateRemoteConfigExcept` never block the calling thread. `stop()` is unchanged and still returns without waiting for network activity. +- Fixed a lost wakeup when stopping the periodic SDK Behavior Settings timer: the stop flag was set without holding the mutex the timer thread waits on, so the notification could be missed and the joining thread could block for up to the full four-hour update interval. ## 26.1.1 - Updated CMake minimum required version to use the range format with upper the end of `3.31`. diff --git a/include/countly.hpp b/include/countly.hpp index 60016b66..1f7c52d5 100644 --- a/include/countly.hpp +++ b/include/countly.hpp @@ -435,11 +435,24 @@ class Countly : public cly::CountlyDelegates { void releaseDatabasePathClaim(); /** - * Joins and clears every remote-config fetch thread. Safe to call repeatedly - * and safe to call when none are running. Must not be called while the - * instance mutex is held -- the fetch threads take it. + * Joins the remote-config fetch thread if one exists. Safe to call repeatedly + * and safe to call when none is running. Must not be called while the instance + * mutex is held -- the fetch thread takes it. */ - void joinRemoteConfigThreads(); + void joinRemoteConfigThread(); + + /** + * Starts a remote-config fetch on the single owned fetch thread. Never blocks + * the caller: if a fetch is already in flight this logs a warning and returns + * false rather than waiting, so a call from a UI thread cannot stall on an + * HTTP timeout. + * + * @param member: the fetch body to run + * @param data: request parameters, copied into the thread + * @param caller: public method name, used in log messages + * @return true if a fetch was started + */ + bool startRemoteConfigThread(void (Countly::*member)(const std::map &), const std::map &data, const char *caller); void _deleteThread(); void _sendIndependantLocationRequest(); @@ -475,10 +488,12 @@ class Countly : public cly::CountlyDelegates { std::unique_ptr thread; - // Remote-config fetches run on owned threads rather than detached ones: a - // detached thread captures `this` and can outlive the instance. + // A remote-config fetch runs on an owned thread rather than a detached one: a + // detached thread captures `this` and can outlive the instance. At most one + // fetch is in flight, so a single thread is all that is ever needed. std::mutex remote_config_thread_mutex; - std::vector remote_config_threads; + std::thread remote_config_thread; + std::atomic remote_config_fetch_running{false}; std::unique_ptr crash_module; std::unique_ptr views_module; diff --git a/include/countly/request_module.hpp b/include/countly/request_module.hpp index c811ac6e..77d3e0a9 100644 --- a/include/countly/request_module.hpp +++ b/include/countly/request_module.hpp @@ -40,24 +40,31 @@ class RequestModule { void setConfigurationProvider(std::weak_ptr provider); // try injecting /** - * Process-wide network initialisation. Idempotent, and a no-op unless the SDK - * is built against libcurl. + * Process-wide network initialisation. Idempotent and harmless to call at any + * time; a no-op unless the SDK is built against libcurl. */ static void initGlobalNetworking(); - /** - * Process-wide network teardown. Idempotent, and a no-op unless the SDK is - * built against libcurl. Must not be called while any instance is live -- - * Countly::shutdownNetworking() enforces that. - */ - static void releaseGlobalNetworking(); - #ifdef COUNTLY_BUILD_TESTS + /** Times curl_global_init actually ran. Structurally at most one. */ static int globalNetworkingInitCount(); + /** Times initGlobalNetworking() was called, i.e. how much was deduplicated. */ + static int globalNetworkingInitRequests(); static bool globalNetworkingReleased(); #endif private: + // Tearing networking down is only safe once no instance is live, and only + // Countly::shutdownNetworking() knows that. Keeping this private stops an + // integrator reaching past the check. + friend class Countly; + + /** + * Process-wide network teardown. Idempotent, and a no-op unless the SDK is + * built against libcurl. + */ + static void releaseGlobalNetworking(); + class RequestModuleImpl; std::unique_ptr impl; std::weak_ptr _configProvider; diff --git a/src/configuration_module.cpp b/src/configuration_module.cpp index 4db1e5b9..adc661f7 100644 --- a/src/configuration_module.cpp +++ b/src/configuration_module.cpp @@ -382,8 +382,17 @@ class ConfigurationModule::ConfigurationModuleImpl { } void _stopTimer() { - _logger->log(LogLevel::INFO, "[Countly] [ConfigurationModule] stopTimer, stopping server config update timer thread."); - stopConfigThread.store(true, std::memory_order_release); + _logger->log(LogLevel::WARNING, "[Countly] [ConfigurationModule] stopTimer, stopping server config update timer thread."); + + { + // The flag has to be mutated under the same mutex the waiter evaluates it + // under. Storing it outside and then notifying can land in the window + // between the waiter checking the predicate and actually blocking, in + // which case the notification is lost -- and the wait deadline is four + // hours, so join() below would block for that long. + std::lock_guard lk(configUpdateMutex); + stopConfigThread.store(true, std::memory_order_release); + } configUpdateCv.notify_all(); if (configUpdateThread.joinable()) { diff --git a/src/countly.cpp b/src/countly.cpp index 5c5d67ad..5bf79f26 100644 --- a/src/countly.cpp +++ b/src/countly.cpp @@ -31,10 +31,33 @@ namespace { * while this is non-zero. */ std::atomic live_instance_count(0); + +/** + * Serialises instance construction and destruction against + * shutdownNetworking(), so that no instance can come into existence between + * that function reading the count and releasing networking. + * + * Lock order: this is acquired *inside* the registry's instance mutex, because + * Countly() is constructed while the registry holds it. shutdownNetworking() + * must therefore finish with the registry before acquiring this. + */ +std::mutex networking_lifecycle_mutex; + +/** + * Clears the in-flight flag however a remote-config fetch body exits. + */ +struct RemoteConfigFetchGuard { + std::atomic &flag; + explicit RemoteConfigFetchGuard(std::atomic &f) : flag(f) {} + ~RemoteConfigFetchGuard() { flag.store(false); } +}; } // namespace Countly::Countly() { - live_instance_count.fetch_add(1); + { + std::lock_guard lk(networking_lifecycle_mutex); + live_instance_count.fetch_add(1); + } crash_module = nullptr; views_module = nullptr; logger.reset(new cly::LoggerModule()); @@ -43,19 +66,39 @@ Countly::Countly() { Countly::~Countly() { is_being_disposed = true; + // Before stop(), because the fetch bodies use requestModule and the instance + // mutex, both of which must still be alive. The SBS timer is joined later by + // configurationModule.reset(), exactly as it was before multi-instance. + joinRemoteConfigThread(); stop(); releaseDatabasePathClaim(); crash_module.reset(); views_module.reset(); configurationModule.reset(); logger.reset(); - live_instance_count.fetch_sub(1); + { + std::lock_guard lk(networking_lifecycle_mutex); + live_instance_count.fetch_sub(1); + } } namespace { const char *const DEFAULT_INSTANCE_NAME = ""; +/** + * Lock-free fast path for the default instance. + * + * getInstance() is the hottest entry point in the SDK -- the sample and the Qt + * demo call it at every call site rather than caching the reference -- and it + * used to be a null check and a dereference. Routing it through the registry + * would have cost a mutex, a linear scan and two atomic refcount operations on + * every call, which is a tax on integrators who never asked for a second + * instance. This pointer is published under the registry lock when the default + * instance is created and cleared when it is destroyed. + */ +std::atomic default_instance_cache(nullptr); + /** * Process-wide state shared by every Countly instance: * - the named-instance table, including the unnamed default instance @@ -79,35 +122,46 @@ class InstanceRegistry { * Returns the instance registered under 'name', creating it if absent. * *created_out reports whether this call created it. */ - std::shared_ptr getOrCreate(const std::string &name, bool *created_out) { - std::shared_ptr fresh; - { - std::lock_guard lk(_instances_mutex); - for (size_t index = 0; index < _instances.size(); index++) { - if (_instances[index].first == name) { - if (created_out != nullptr) { - *created_out = false; - } - return _instances[index].second; + /** + * Returns a raw pointer rather than a shared_ptr on purpose. Handing ownership + * out would mean destroyInstance() could return while another thread still + * held a reference, leaving the instance -- and its database path claim -- + * alive after the call that was supposed to free it. The registry is the sole + * owner; the returned pointer is valid until that entry is destroyed, which is + * exactly the lifetime the public API documents. + */ + Countly *getOrCreate(const std::string &name, bool *created_out) { + std::lock_guard lk(_instances_mutex); + for (size_t index = 0; index < _instances.size(); index++) { + if (_instances[index].first == name) { + if (created_out != nullptr) { + *created_out = false; } + return _instances[index].second.get(); } - fresh.reset(new Countly()); - _instances.push_back(std::make_pair(name, fresh)); } + + // Countly() takes networking_lifecycle_mutex, so the lock order here is + // instances -> networking. Nothing may acquire them the other way round. + std::shared_ptr fresh(new Countly()); + _instances.push_back(std::make_pair(name, fresh)); if (created_out != nullptr) { *created_out = true; } - return fresh; + if (name.empty()) { + default_instance_cache.store(fresh.get(), std::memory_order_release); + } + return fresh.get(); } - std::shared_ptr find(const std::string &name) { + Countly *find(const std::string &name) { std::lock_guard lk(_instances_mutex); for (size_t index = 0; index < _instances.size(); index++) { if (_instances[index].first == name) { - return _instances[index].second; + return _instances[index].second.get(); } } - return std::shared_ptr(); + return nullptr; } /** @@ -121,6 +175,9 @@ class InstanceRegistry { if (it->first == name) { std::shared_ptr found = it->second; _instances.erase(it); + if (name.empty()) { + default_instance_cache.store(nullptr, std::memory_order_release); + } return found; } } @@ -132,6 +189,7 @@ class InstanceRegistry { std::vector> doomed; { std::lock_guard lk(_instances_mutex); + default_instance_cache.store(nullptr, std::memory_order_release); for (size_t index = _instances.size(); index > 0; index--) { doomed.push_back(_instances[index - 1].second); } @@ -176,10 +234,11 @@ InstanceRegistry ®istry() { } // namespace void Countly::inheritDefaultLogger(Countly &instance) { - std::shared_ptr defaultInstance = registry().find(DEFAULT_INSTANCE_NAME); - if (!defaultInstance || defaultInstance.get() == &instance) { + Countly *defaultInstance = registry().find(DEFAULT_INSTANCE_NAME); + if (defaultInstance == nullptr || defaultInstance == &instance) { return; } + // LoggerModule serialises this against a concurrent setLogger(). LoggerFunction callback = defaultInstance->logger->getLogger(); if (callback != nullptr) { instance.logger->setLogger(callback); @@ -195,11 +254,20 @@ void Countly::releaseDatabasePathClaim() { #endif } -Countly &Countly::getInstance() { return getInstance(DEFAULT_INSTANCE_NAME); } +Countly &Countly::getInstance() { + // Lock-free once the default instance exists, which is the overwhelmingly + // common case. Only the very first call, or the first after halt() or + // destroyInstance(""), pays for the registry lookup. + Countly *cached = default_instance_cache.load(std::memory_order_acquire); + if (cached != nullptr) { + return *cached; + } + return getInstance(DEFAULT_INSTANCE_NAME); +} Countly &Countly::getInstance(const std::string &name) { bool created = false; - std::shared_ptr instance = registry().getOrCreate(name, &created); + Countly *instance = registry().getOrCreate(name, &created); if (created && !name.empty()) { inheritDefaultLogger(*instance); instance->log(LogLevel::WARNING, "[Countly] getInstance, creating a new, uninitialized instance named '" + name + "'; if you expected an existing instance, check the name."); @@ -209,7 +277,7 @@ Countly &Countly::getInstance(const std::string &name) { Countly &Countly::createInstance(const std::string &name) { bool created = false; - std::shared_ptr instance = registry().getOrCreate(name, &created); + Countly *instance = registry().getOrCreate(name, &created); if (created) { inheritDefaultLogger(*instance); } else { @@ -218,10 +286,7 @@ Countly &Countly::createInstance(const std::string &name) { return *instance; } -Countly *Countly::findInstance(const std::string &name) { - std::shared_ptr instance = registry().find(name); - return instance ? instance.get() : nullptr; -} +Countly *Countly::findInstance(const std::string &name) { return registry().find(name); } bool Countly::hasInstance(const std::string &name) { return findInstance(name) != nullptr; } @@ -233,17 +298,26 @@ void Countly::destroyInstance(const std::string &name) { void Countly::destroyAllInstances() { registry().clear(); } void Countly::shutdownNetworking() { - const int live = live_instance_count.load(); - if (live > 0) { - // Log through the default instance if one exists -- a diagnostic must not - // create an instance as a side effect. - std::shared_ptr defaultInstance = registry().find(DEFAULT_INSTANCE_NAME); - if (defaultInstance) { - defaultInstance->log(LogLevel::ERROR, "[Countly] shutdownNetworking, " + std::to_string(live) + " instance(s) are still live; refusing to tear down networking."); + // Resolve the log target before taking networking_lifecycle_mutex. Countly() + // is constructed while the registry holds its instance mutex and then takes + // the networking mutex, so acquiring them in the opposite order here would + // deadlock. Finish with the registry first, then lock. + Countly *defaultInstance = registry().find(DEFAULT_INSTANCE_NAME); + + int live = 0; + { + // Held across the check and the release so no instance can be constructed + // in between and have networking torn down underneath it. + std::lock_guard lk(networking_lifecycle_mutex); + live = live_instance_count.load(); + if (live == 0) { + RequestModule::releaseGlobalNetworking(); } - return; } - RequestModule::releaseGlobalNetworking(); + + if (live > 0 && defaultInstance != nullptr) { + defaultInstance->log(LogLevel::ERROR, "[Countly] shutdownNetworking, " + std::to_string(live) + " instance(s) are still live; refusing to tear down networking."); + } } #ifdef COUNTLY_BUILD_TESTS @@ -729,7 +803,8 @@ void Countly::start(const std::string &app_key, const std::string &host, int por // with the wrong key. const std::string normalized_database_path = cly::utils::normalizeDatabasePath(configuration->databasePath); if (!registry().tryClaimPath(normalized_database_path)) { - log(LogLevel::ERROR, "[Countly] start, Database path '" + configuration->databasePath + "' is already in use by another Countly instance in this process. SDK will not be initialized."); + log(LogLevel::ERROR, "[Countly] start, Database path '" + configuration->databasePath + + "' is already in use by another Countly instance in this process. Note that stop() does not free the path: an instance keeps its claim until it is destroyed. SDK will not be initialized."); return; } claimed_database_path = normalized_database_path; @@ -845,28 +920,48 @@ void Countly::startOnCloud(const std::string &app_key) { this->start(app_key, "https://cloud.count.ly", 443); } -void Countly::joinRemoteConfigThreads() { +void Countly::joinRemoteConfigThread() { std::lock_guard lk(remote_config_thread_mutex); - for (size_t index = 0; index < remote_config_threads.size(); index++) { - if (remote_config_threads[index].joinable()) { - try { - remote_config_threads[index].join(); - } catch (const std::system_error &e) { - log(LogLevel::WARNING, std::string("[Countly] joinRemoteConfigThreads, Could not join thread: ") + e.what()); - } + if (remote_config_thread.joinable()) { + try { + remote_config_thread.join(); + } catch (const std::system_error &e) { + log(LogLevel::WARNING, std::string("[Countly] joinRemoteConfigThread, Could not join thread: ") + e.what()); } } - remote_config_threads.clear(); } -void Countly::stop() { - // configurationModule is only constructed inside start(), so it is null on an - // instance that was never started -- createInstance() followed by - // destroyInstance(), or a start() that was refused. Guard every call into it. - if (configurationModule) { - configurationModule->stopTimer(); +bool Countly::startRemoteConfigThread(void (Countly::*member)(const std::map &), const std::map &data, const char *caller) { + std::lock_guard lk(remote_config_thread_mutex); + + if (remote_config_fetch_running.load()) { + log(LogLevel::WARNING, std::string("[Countly] ") + caller + ", a remote config fetch is already in flight; ignoring this call."); + return false; + } + + // The previous fetch has finished, so this returns immediately. Joining it is + // what lets a single std::thread member serve every fetch. + if (remote_config_thread.joinable()) { + remote_config_thread.join(); + } + + remote_config_fetch_running.store(true); + try { + remote_config_thread = std::thread(member, this, data); + } catch (const std::system_error &e) { + remote_config_fetch_running.store(false); + log(LogLevel::ERROR, std::string("[Countly] ") + caller + ", could not create remote config thread: " + e.what()); + return false; } - joinRemoteConfigThreads(); + return true; +} + +void Countly::stop() { + // Deliberately unchanged from before multi-instance support. Joining the SDK + // Behavior Settings timer or an in-flight remote-config fetch here would make + // stop() block where it previously returned at once, and integrators call it + // on the UI thread at shutdown (see examples/qt_demo). Both joins belong in + // ~Countly, which is where the lifetime problem they solve actually arises. _deleteThread(); if (configuration->manualSessionControl == false) { endSession(); @@ -1791,6 +1886,8 @@ void Countly::enableRemoteConfig() { } void Countly::_fetchRemoteConfig(const std::map &data) { + RemoteConfigFetchGuard fetch_guard(remote_config_fetch_running); + if (configurationModule->isNetworkingEnabled() == false) { log(LogLevel::ERROR, "[Countly] _fetchRemoteConfig, Error fetching remote config, networking is disabled in SBS"); return; @@ -1818,14 +1915,9 @@ void Countly::updateRemoteConfig() { lk.unlock(); - // Fetch remote config asynchronously on an owned thread. Any previous fetch is - // joined first, so back-to-back calls serialise rather than piling up threads - // that capture `this`. - joinRemoteConfigThreads(); - { - std::lock_guard tlk(remote_config_thread_mutex); - remote_config_threads.push_back(std::thread(&Countly::_fetchRemoteConfig, this, data)); - } + // Fetch remote config asynchronously on the owned fetch thread. Never blocks: + // if a fetch is already running this logs and returns. + startRemoteConfigThread(&Countly::_fetchRemoteConfig, data, "updateRemoteConfig"); } nlohmann::json Countly::getRemoteConfigValue(const std::string &key) { @@ -1835,6 +1927,8 @@ nlohmann::json Countly::getRemoteConfigValue(const std::string &key) { } void Countly::_updateRemoteConfigWithSpecificValues(const std::map &data) { + RemoteConfigFetchGuard fetch_guard(remote_config_fetch_running); + if (configurationModule->isNetworkingEnabled() == false) { log(LogLevel::ERROR, "[Countly] _updateRemoteConfigWithSpecificValues, Error fetching remote config, networking is disabled in SBS"); return; @@ -1866,14 +1960,9 @@ void Countly::updateRemoteConfigFor(std::string *keys, size_t key_count) { } lk.unlock(); - // Fetch remote config asynchronously on an owned thread. Any previous fetch is - // joined first, so back-to-back calls serialise rather than piling up threads - // that capture `this`. - joinRemoteConfigThreads(); - { - std::lock_guard tlk(remote_config_thread_mutex); - remote_config_threads.push_back(std::thread(&Countly::_updateRemoteConfigWithSpecificValues, this, data)); - } + // Fetch remote config asynchronously on the owned fetch thread. Never blocks: + // if a fetch is already running this logs and returns. + startRemoteConfigThread(&Countly::_updateRemoteConfigWithSpecificValues, data, "updateRemoteConfigFor"); } void Countly::updateRemoteConfigExcept(std::string *keys, size_t key_count) { @@ -1893,13 +1982,8 @@ void Countly::updateRemoteConfigExcept(std::string *keys, size_t key_count) { } lk.unlock(); - // Fetch remote config asynchronously on an owned thread. Any previous fetch is - // joined first, so back-to-back calls serialise rather than piling up threads - // that capture `this`. - joinRemoteConfigThreads(); - { - std::lock_guard tlk(remote_config_thread_mutex); - remote_config_threads.push_back(std::thread(&Countly::_updateRemoteConfigWithSpecificValues, this, data)); - } + // Fetch remote config asynchronously on the owned fetch thread. Never blocks: + // if a fetch is already running this logs and returns. + startRemoteConfigThread(&Countly::_updateRemoteConfigWithSpecificValues, data, "updateRemoteConfigExcept"); } } // namespace cly diff --git a/src/logger_module.cpp b/src/logger_module.cpp index a44fc782..c5187ac7 100644 --- a/src/logger_module.cpp +++ b/src/logger_module.cpp @@ -1,10 +1,17 @@ #include "countly/logger_module.hpp" +#include #include #include +#include namespace cly { class LoggerModule::LoggerModuleImpl { public: LoggerModuleImpl() {} + std::mutex mutex; + // Mirrors whether logger_function is set, so log() can bail out without + // touching the mutex. Most integrators never register a callback, and the SDK + // logs at DEBUG level on per-event paths. + std::atomic has_logger{false}; LoggerFunction logger_function; }; @@ -12,13 +19,36 @@ LoggerModule::LoggerModule() { impl = std::make_unique(); } LoggerModule::~LoggerModule() {} -void LoggerModule::setLogger(LoggerFunction logger) { impl->logger_function = logger; } +void LoggerModule::setLogger(LoggerFunction logger) { + std::lock_guard lk(impl->mutex); + impl->logger_function = logger; + impl->has_logger.store(impl->logger_function != nullptr, std::memory_order_release); +} -const LoggerFunction LoggerModule::getLogger() { return impl->logger_function; } +const LoggerFunction LoggerModule::getLogger() { + std::lock_guard lk(impl->mutex); + return impl->logger_function; +} void LoggerModule::log(LogLevel level, const std::string &message) { - if (impl->logger_function != nullptr) { - impl->logger_function(level, message); + // Fast path for the common case of no registered callback: one atomic load and + // out, no lock. Without this, adding the mutex would have made every DEBUG log + // site on the per-event paths more expensive than before it existed. + if (!impl->has_logger.load(std::memory_order_acquire)) { + return; + } + + // Copy the callback under the lock and invoke it outside: the callback belongs + // to the integrator and may call back into the SDK, which would re-enter this + // function and deadlock on a non-recursive mutex. + LoggerFunction callback; + { + std::lock_guard lk(impl->mutex); + callback = impl->logger_function; + } + + if (callback != nullptr) { + callback(level, message); } } } // namespace cly diff --git a/src/request_module.cpp b/src/request_module.cpp index ed8aca24..1b220706 100644 --- a/src/request_module.cpp +++ b/src/request_module.cpp @@ -128,15 +128,28 @@ CurlGlobal &curlGlobal() { static CurlGlobal instance; return instance; } + +/** + * How many times initGlobalNetworking() was asked to initialise, as opposed to + * how many times curl actually was. The gap between the two is the whole point + * of CurlGlobal, so both numbers are needed to assert that deduplication works + * -- the actual count is 1 by construction and proves nothing on its own. + */ +std::atomic networking_init_requests(0); } // namespace -void RequestModule::initGlobalNetworking() { curlGlobal(); } +void RequestModule::initGlobalNetworking() { + networking_init_requests.fetch_add(1); + curlGlobal(); +} void RequestModule::releaseGlobalNetworking() { curlGlobal().release(); } #ifdef COUNTLY_BUILD_TESTS int RequestModule::globalNetworkingInitCount() { return curlGlobal().initCount(); } +int RequestModule::globalNetworkingInitRequests() { return networking_init_requests.load(); } + bool RequestModule::globalNetworkingReleased() { return curlGlobal().released(); } #endif diff --git a/tests/multi_instance.cpp b/tests/multi_instance.cpp index a547d5ce..44d2105c 100644 --- a/tests/multi_instance.cpp +++ b/tests/multi_instance.cpp @@ -143,11 +143,15 @@ TEST_CASE("curl global state is initialised once and survives instance destructi clearSDK(); InstanceFixture a("APP_KEY_A", "mi-curl-a.db"); REQUIRE(a.initialized()); - CHECK(cly::RequestModule::globalNetworkingInitCount() == 1); { InstanceFixture b("APP_KEY_B", "mi-curl-b.db"); REQUIRE(b.initialized()); + + // The point of CurlGlobal is the gap between these two numbers: several + // requests to initialise, exactly one initialisation. Asserting the actual + // count alone would be vacuous -- it is 1 by construction. + CHECK(cly::RequestModule::globalNetworkingInitRequests() >= 2); CHECK(cly::RequestModule::globalNetworkingInitCount() == 1); } @@ -157,6 +161,56 @@ TEST_CASE("curl global state is initialised once and survives instance destructi CHECK(cly::RequestModule::globalNetworkingInitCount() == 1); } +TEST_CASE("a second remote config fetch is dropped rather than blocking the caller") { + clearSDK(); + static std::atomic fetches_started(0); + static std::atomic release_fetch(false); + fetches_started.store(0); + release_fetch.store(false); + + std::shared_ptr sdk = std::make_shared(); + sdk->setHTTPClient([](bool use_post, const std::string &url, const std::string &data) { + (void)use_post; + (void)url; + cly::HTTPResponse response; + response.success = true; + response.data = nlohmann::json::object(); + if (data.find("fetch_remote_config") != std::string::npos) { + fetches_started.fetch_add(1); + while (!release_fetch.load()) { + std::this_thread::sleep_for(std::chrono::milliseconds(5)); + } + } + return response; + }); + sdk->setDeviceID(COUNTLY_TEST_DEVICE_ID); + sdk->SetPath("mi-rc-drop.db"); + sdk->disableSDKBehaviorSettingsUpdates(); + sdk->enableImmediateRequestOnStop(); + sdk->enableRemoteConfig(); + sdk->start("APP_KEY_RC2", COUNTLY_TEST_HOST, COUNTLY_TEST_PORT, false); + REQUIRE(sdk->checkEQSize() == 0); + + sdk->updateRemoteConfig(); + // Wait for the first fetch to be genuinely in flight and parked. + for (int i = 0; i < 200 && fetches_started.load() == 0; i++) { + std::this_thread::sleep_for(std::chrono::milliseconds(5)); + } + REQUIRE(fetches_started.load() == 1); + + // The second call must return immediately instead of waiting for the first. + const std::chrono::steady_clock::time_point before = std::chrono::steady_clock::now(); + sdk->updateRemoteConfig(); + const long long elapsed_ms = std::chrono::duration_cast(std::chrono::steady_clock::now() - before).count(); + + CHECK(elapsed_ms < 200); // did not block on the parked fetch + CHECK(fetches_started.load() == 1); // and did not start a second one + + release_fetch.store(true); + sdk.reset(); + remove("mi-rc-drop.db"); +} + TEST_CASE("named instances are distinct objects and are found by name") { clearSDK(); cly::Countly &a = cly::Countly::createInstance("appA"); From 350edacdf3d39a714d1d2271da1867e8360b1a32 Mon Sep 17 00:00:00 2001 From: turtledreams <62231246+turtledreams@users.noreply.github.com> Date: Wed, 5 Aug 2026 20:39:22 +0900 Subject: [PATCH 3/5] ch --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0c8c5c5c..35590f97 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,7 @@ - ! Minor breaking change ! When built with SQLite, each instance requires its own database path. A second instance claiming a path already in use logs an error and does not initialize. - Fixed the libcurl global lifecycle: `curl_global_init` now runs once per process, and cleanup no longer runs when an instance is destroyed, which could tear down networking underneath another live instance. Added `shutdownNetworking()` for hosts that load and unload the SDK without exiting. - Fixed non-unique event and view IDs: the random component of generated IDs was constant for the lifetime of the process, and on platforms with a coarse `system_clock` (Windows, ~15ms) the timestamp component did not change either, so IDs generated within one tick were identical. -- Remote config fetches now run on an owned thread that is joined when the SDK is destroyed, instead of being detached, which could leave a fetch running after the objects it used were gone. Only one fetch runs at a time per instance: a call made while a fetch is in flight is logged and ignored rather than queued, so `updateRemoteConfig`, `updateRemoteConfigFor` and `updateRemoteConfigExcept` never block the calling thread. `stop()` is unchanged and still returns without waiting for network activity. +- Remote config fetches now run on an owned thread that is joined when the SDK is destroyed, instead of being detached, which could leave a fetch running after the objects it used were gone. Only one fetch runs at a time per instance: a call made while a fetch is in flight is logged and ignored rather than queued, so `updateRemoteConfig`, `updateRemoteConfigFor` and `updateRemoteConfigExcept` never block the calling thread. - Fixed a lost wakeup when stopping the periodic SDK Behavior Settings timer: the stop flag was set without holding the mutex the timer thread waits on, so the notification could be missed and the joining thread could block for up to the full four-hour update interval. ## 26.1.1 From 4277ee774023ab9ee0d4f6ad3812b1f44d6a93cd Mon Sep 17 00:00:00 2001 From: turtledreams Date: Fri, 7 Aug 2026 18:06:35 +0900 Subject: [PATCH 4/5] example --- CHANGELOG.md | 1 + examples/example_integration.cpp | 235 ++++++++++++++++++++++++++++--- include/countly/constants.hpp | 31 ++++ src/event.cpp | 5 +- src/request_builder.cpp | 7 +- tests/multi_instance.cpp | 35 +++++ 6 files changed, 294 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 35590f97..8c930db4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ - Fixed the libcurl global lifecycle: `curl_global_init` now runs once per process, and cleanup no longer runs when an instance is destroyed, which could tear down networking underneath another live instance. Added `shutdownNetworking()` for hosts that load and unload the SDK without exiting. - Fixed non-unique event and view IDs: the random component of generated IDs was constant for the lifetime of the process, and on platforms with a coarse `system_clock` (Windows, ~15ms) the timestamp component did not change either, so IDs generated within one tick were identical. - Remote config fetches now run on an owned thread that is joined when the SDK is destroyed, instead of being detached, which could leave a fetch running after the objects it used were gone. Only one fetch runs at a time per instance: a call made while a fetch is in flight is logged and ignored rather than queued, so `updateRemoteConfig`, `updateRemoteConfigFor` and `updateRemoteConfigExcept` never block the calling thread. +- Fixed a data race in the `dow`, `hour` and `tz` fields of events and requests: they were derived with `std::localtime` and `std::gmtime`, which share one process-wide buffer, so concurrent recording (each instance runs its own update loop) could report another thread's or the other function's values. Both now use the reentrant variants. - Fixed a lost wakeup when stopping the periodic SDK Behavior Settings timer: the stop flag was set without holding the mutex the timer thread waits on, so the notification could be missed and the joining thread could block for up to the full four-hour update interval. ## 26.1.1 diff --git a/examples/example_integration.cpp b/examples/example_integration.cpp index 2189ae5c..ff598ca7 100644 --- a/examples/example_integration.cpp +++ b/examples/example_integration.cpp @@ -1,8 +1,10 @@ #include "countly.hpp" +#include #include #include #include #include +#include using namespace std; using namespace cly; @@ -139,9 +141,40 @@ void printLog(LogLevel level, const string &msg) { // return response; // } -int main() { - cout << "Sample App" << endl; - Countly &ct = Countly::getInstance(); +// --------------------------------------------------------------------------- +// Multi instance support +// +// The SDK keeps a process wide registry of named instances: +// * the unnamed instance is the default one, returned by Countly::getInstance() +// * any other instance is created with Countly::createInstance(name) and later +// looked up with Countly::getInstance(name) or Countly::findInstance(name) +// +// Each instance is fully independent: its own app key, device id, session, +// event queue, request queue, background thread and modules. Two rules matter +// when you run more than one: +// 1. Give every instance its own app key. +// 2. On SQLite builds give every instance its own database file. start() +// refuses a path that another live instance has already claimed, and the +// claim is only released when that instance is destroyed (not on stop()). +// --------------------------------------------------------------------------- +static const string SERVER_URL = "https://your.server.ly"; +static const int SERVER_PORT = 443; + +static const string PRIMARY_APP_KEY = "YOUR_APP_KEY"; +static const string PRIMARY_DEVICE_ID = "test-device-id"; +static const string PRIMARY_DB_PATH = "databaseFileName.db"; + +// The name is just a registry key, it is never sent to the server. +static const string SECONDARY_INSTANCE_NAME = "secondary"; +static const string SECONDARY_APP_KEY = "YOUR_SECOND_APP_KEY"; +static const string SECONDARY_DEVICE_ID = "test-device-id-2"; +static const string SECONDARY_DB_PATH = "databaseFileName2.db"; + +// Applies the same set of configurations to any instance and starts it. +// Note that every setter is called on the instance it belongs to: there is no +// "current" instance in the SDK, so a call on the default instance never +// configures a named one. +static void configureAndStart(Countly &instance, const string &appKey, const string &deviceId, const string &dbPath) { // All configurations below are put here as an example // Your configuration in your app may be different // Please refer to the documentation for more information: @@ -149,27 +182,132 @@ int main() { // Custom HTTP client // HTTPClientFunction clientPtr = customClient; - // ct.setHTTPClient(clientPtr); - // ct.alwaysUsePost(true); - ct.setLogger(printLog); - ct.SetPath("databaseFileName.db"); // this will be only built into account if the correct configurations are set - ct.setDeviceID("test-device-id"); - // ct.setSalt("test-salt"); + // instance.setHTTPClient(clientPtr); + // instance.alwaysUsePost(true); + instance.setLogger(printLog); + instance.SetPath(dbPath); // this will be only built into account if the correct configurations are set + instance.setDeviceID(deviceId); + // instance.setSalt("test-salt"); // OS, OS_version, device, resolution, carrier, app_version); - ct.SetMetrics("Windows 10", "10.22", "Mac", "800x600", "Carrier", "1.0"); + instance.SetMetrics("Windows 10", "10.22", "Mac", "800x600", "Carrier", "1.0"); + + instance.setAutomaticSessionUpdateInterval(5); // The value is set so low just for internal validation. Has to be set before start. + instance.setMaxRQProcessingBatchSize(2); // in most cases not needed to be set. The value is set so low just for internal validation // start the SDK (initialize the SDK) - string _appKey = "YOUR_APP_KEY"; - string _serverUrl = "https://your.server.ly"; + instance.start(appKey, SERVER_URL, SERVER_PORT, true); +} + +// Creates and starts the secondary instance. createInstance returns the existing +// instance (and logs a warning) if the name is already taken, so this is safe to +// call twice. +static Countly &startSecondaryInstance() { + Countly &second = Countly::createInstance(SECONDARY_INSTANCE_NAME); + configureAndStart(second, SECONDARY_APP_KEY, SECONDARY_DEVICE_ID, SECONDARY_DB_PATH); + return second; +} + +// Always resolve a named instance through findInstance instead of caching the +// reference: destroyInstance() frees the object, and any reference or pointer +// kept across that call dangles. +static Countly *secondaryInstance() { + Countly *second = Countly::findInstance(SECONDARY_INSTANCE_NAME); + if (second == nullptr) { + printLog(LogLevel::WARNING, "[ExampleIntegration] The secondary instance does not exist, create it first"); + } + return second; +} + +// Hammers both instances from several threads at once. Each instance has its own +// lock, so the two do not contend with each other; within one instance the calls +// below are internally synchronised. +// +// Views are deliberately driven from a single thread per instance: ViewsModule +// keeps its open-view map without a lock of its own, so concurrent openView / +// closeView calls on the *same* instance are not safe. Two threads driving views +// on two *different* instances are, since the modules are per instance. +static void stressBothInstances(int threadsPerInstance, int iterations) { + Countly *second = secondaryInstance(); + if (second == nullptr) { + return; + } + + Countly *targets[2] = {&Countly::getInstance(), second}; + const char *labels[2] = {"primary", "secondary"}; + + std::vector workers; + std::atomic recorded(0); + + for (int t = 0; t < 2; t++) { + Countly *target = targets[t]; + const string label = labels[t]; + + for (int w = 0; w < threadsPerInstance; w++) { + workers.emplace_back([target, label, w, iterations, &recorded]() { + for (int i = 0; i < iterations; i++) { + target->RecordEvent("stress_basic_" + label, 1); + + std::map segmentation = { + {"instance", label}, + {"worker", std::to_string(w)}, + {"iteration", std::to_string(i)}, + }; + target->RecordEvent("stress_segmented_" + label, segmentation, 1, 2.5, 0.5); + + target->crash().addBreadcrumb(label + "-" + std::to_string(w) + "-" + std::to_string(i)); + + if (i % 5 == 0) { + target->updateSession(); + } + + recorded.fetch_add(2); + } + }); + } - if(_appKey.compare("YOUR_APP_KEY") == 0 || _serverUrl.compare("https://your.server.ly") == 0) { + // One view thread per instance, see the note above. + workers.emplace_back([target, label, iterations]() { + for (int i = 0; i < iterations; i++) { + const std::string viewId = target->views().openView("stress view " + label); + if (!viewId.empty()) { + target->views().closeViewWithID(viewId); + } + } + }); + } + + for (std::thread &worker : workers) { + worker.join(); + } + + printLog(LogLevel::INFO, "[ExampleIntegration] Stress finished, events recorded = " + std::to_string(recorded.load()) + ", primary EQ = " + std::to_string(Countly::getInstance().checkEQSize()) + ", secondary EQ = " + std::to_string(second->checkEQSize())); +} + +static void printInstanceStatus() { + Countly *second = Countly::findInstance(SECONDARY_INSTANCE_NAME); + cout << "Default instance : initialized, RQ size = " << Countly::getInstance().checkRQSize() << ", EQ size = " << Countly::getInstance().checkEQSize() << endl; + cout << "Instance '" << SECONDARY_INSTANCE_NAME << "': " << (second == nullptr ? "not created" : "present") << endl; + if (second != nullptr) { + cout << " RQ size = " << second->checkRQSize() << ", EQ size = " << second->checkEQSize() << endl; + } + cout << "hasInstance(\"" << SECONDARY_INSTANCE_NAME << "\") = " << (Countly::hasInstance(SECONDARY_INSTANCE_NAME) ? "true" : "false") << endl; + // getInstance("") is the default instance, so this is always true. + cout << "getInstance(\"\") == getInstance() : " << (&Countly::getInstance("") == &Countly::getInstance() ? "true" : "false") << endl; +} + +int main() { + cout << "Sample App" << endl; + + if (PRIMARY_APP_KEY.compare("YOUR_APP_KEY") == 0 || SERVER_URL.compare("https://your.server.ly") == 0) { printLog(LogLevel::WARNING, "[ExampleIntegration] Please do not use default set of app key and server url"); } - ct.start(_appKey, _serverUrl, 443, true); + // The default (unnamed) instance. + Countly &ct = Countly::getInstance(); + configureAndStart(ct, PRIMARY_APP_KEY, PRIMARY_DEVICE_ID, PRIMARY_DB_PATH); - ct.setAutomaticSessionUpdateInterval(5);// The value is set so low just for internal validation. - ct.setMaxRQProcessingBatchSize(2); // in most cases not needed to be set. The value is set so low just for internal validation + // A second, independent instance reporting to a second app. + startSecondaryInstance(); ct.crash().addBreadcrumb("start"); @@ -189,6 +327,14 @@ int main() { cout << "11) Record a view" << endl; cout << "12) Leave breadcrumb" << endl; cout << "13) Record a crash with bread crumbs and segmentation" << endl; + cout << "-- multi instance --" << endl; + cout << "14) Basic event on the secondary instance" << endl; + cout << "15) Same event on both instances" << endl; + cout << "16) Record a view on both instances" << endl; + cout << "17) Concurrency stress on both instances" << endl; + cout << "18) Instance registry status" << endl; + cout << "19) Destroy the secondary instance" << endl; + cout << "20) Create and start the secondary instance again" << endl; cout << "0) Exit" << endl; int a; cin >> a; @@ -222,7 +368,9 @@ int main() { {"name", "Full name"}, {"username", "username123"}, {"email", "useremail@email.com"}, {"phone", "222-222-222"}, {"phone", "222-222-222"}, {"picture", "http://webresizer.com/images2/bird1_after.jpg"}, {"gender", "M"}, {"byear", "1991"}, {"organization", "Organization"}, }; - ct.getInstance().setUserDetails(userdetail); + // Call the setter on the instance you mean. 'ct.getInstance()' would work + // here only because 'ct' happens to be the default instance. + ct.setUserDetails(userdetail); } break; case 8: ct.setDeviceID("new-device-id", true); @@ -272,6 +420,51 @@ int main() { ct.crash().recordException("Divided by zero", "stack trace", true, crashMetrics, segmentation); } break; + case 14: { + if (Countly *second = secondaryInstance()) { + second->RecordEvent("Event on the secondary instance", 1); + } + } break; + case 15: { + // The same key on both instances. Each event stays in the queue of the + // instance it was recorded on and is sent with that instance's app key. + std::map segmentation = {{"source", "menu 15"}}; + ct.RecordEvent("Event on both instances", segmentation, 1); + if (Countly *second = secondaryInstance()) { + second->RecordEvent("Event on both instances", segmentation, 1); + } + } break; + case 16: { + Countly *second = secondaryInstance(); + // View ids are unique per view, so the two instances report different ids + // for the same view name. + const std::string primaryViewId = ct.views().openView("Shared view name"); + const std::string secondaryViewId = second != nullptr ? second->views().openView("Shared view name") : ""; + cout << "primary view id = " << primaryViewId << endl; + cout << "secondary view id = " << secondaryViewId << endl; + + std::this_thread::sleep_for(2s); + + ct.views().closeViewWithID(primaryViewId); + if (second != nullptr && !secondaryViewId.empty()) { + second->views().closeViewWithID(secondaryViewId); + } + } break; + case 17: + stressBothInstances(4, 25); + break; + case 18: + printInstanceStatus(); + break; + case 19: + // Ends the instance's session, joins its threads and frees its database + // path claim. Any reference to it dangles afterwards. + Countly::destroyInstance(SECONDARY_INSTANCE_NAME); + printLog(LogLevel::INFO, "[ExampleIntegration] Secondary instance destroyed"); + break; + case 20: + startSecondaryInstance(); + break; case 0: flag = false; break; @@ -281,7 +474,15 @@ int main() { } } + // Stop every instance, then let the registry destroy them. stop() ends the + // session and joins the update thread; destroyAllInstances() also frees the + // database path claims. 'ct' must not be touched after that call, it refers to + // a destroyed object. + if (Countly *second = Countly::findInstance(SECONDARY_INSTANCE_NAME)) { + second->stop(); + } ct.stop(); + Countly::destroyAllInstances(); return 0; } diff --git a/include/countly/constants.hpp b/include/countly/constants.hpp index e7bbe7f6..b8b34402 100644 --- a/include/countly/constants.hpp +++ b/include/countly/constants.hpp @@ -5,6 +5,7 @@ #include #include #include +#include #include #include #include @@ -54,6 +55,36 @@ template static std::string format_string(const std::string & return str; } +/** + * Thread-safe replacements for std::localtime and std::gmtime. + * + * Both of those return a pointer into a single process-wide std::tm, so two + * threads calling them concurrently race, and a caller can end up copying the + * struct another thread has just overwritten -- including the other function's + * result, since localtime and gmtime share that one buffer. With more than one + * SDK instance this needs no threads of the integrator's own: every instance + * runs its own update loop, and each one builds requests. + */ +inline std::tm localTime(std::time_t time) { + std::tm result = std::tm(); +#if defined(_WIN32) && (defined(_MSC_VER) || defined(MINGW_HAS_SECURE_API)) + localtime_s(&result, &time); +#else + localtime_r(&time, &result); +#endif + return result; +} + +inline std::tm gmTime(std::time_t time) { + std::tm result = std::tm(); +#if defined(_WIN32) && (defined(_MSC_VER) || defined(MINGW_HAS_SECURE_API)) + gmtime_s(&result, &time); +#else + gmtime_r(&time, &result); +#endif + return result; +} + /** * Gives a string representation of the size of a map. * diff --git a/src/event.cpp b/src/event.cpp index 252a6a6a..5f87be03 100644 --- a/src/event.cpp +++ b/src/event.cpp @@ -1,4 +1,5 @@ #include "countly/event.hpp" +#include "countly/constants.hpp" #include "countly/internal_limits.hpp" #include @@ -30,7 +31,9 @@ void Event::setTimestamp() { object["timestamp"] = std::chrono::duration_cast(timestamp.time_since_epoch()).count(); std::time_t time = std::chrono::system_clock::to_time_t(timestamp); - std::tm local_tm = *std::localtime(&time); + // Not std::localtime: it hands back a pointer to one process-wide std::tm, so + // two threads creating events at the same time race on it. + std::tm local_tm = cly::utils::localTime(time); object["dow"] = local_tm.tm_wday; object["hour"] = local_tm.tm_hour; } diff --git a/src/request_builder.cpp b/src/request_builder.cpp index b391a5d5..d6506f3d 100644 --- a/src/request_builder.cpp +++ b/src/request_builder.cpp @@ -28,8 +28,11 @@ std::string RequestBuilder::buildRequest(const std::map(now.time_since_epoch()); std::time_t time = std::chrono::system_clock::to_time_t(now); - std::tm local_tm = *std::localtime(&time); - std::tm gm_tm = *std::gmtime(&time); + // Not std::localtime/std::gmtime: they share one process-wide std::tm, so a + // concurrent request build (each instance has its own update loop) can leave + // local_tm holding GMT fields and silently corrupt tz/dow/hour. + std::tm local_tm = cly::utils::localTime(time); + std::tm gm_tm = cly::utils::gmTime(time); int tz_offset_minutes = (local_tm.tm_hour - gm_tm.tm_hour) * 60 + (local_tm.tm_min - gm_tm.tm_min); // Adjust for day boundary crossings diff --git a/tests/multi_instance.cpp b/tests/multi_instance.cpp index 44d2105c..dc4b3764 100644 --- a/tests/multi_instance.cpp +++ b/tests/multi_instance.cpp @@ -1,8 +1,11 @@ +#include #include #include +#include #include #include #include +#include #include "doctest.h" @@ -64,6 +67,38 @@ TEST_CASE("generateEventID varies its random component") { CHECK(ids.size() == 1000); } +TEST_CASE("localTime and gmTime are safe to call from several threads") { + // Event::setTimestamp and RequestBuilder::buildRequest used std::localtime and + // std::gmtime, which return a pointer into one process-wide std::tm. Two + // threads racing there could copy a struct the other had already overwritten, + // and because localtime and gmtime share the buffer a "local" tm could come + // back holding GMT fields -- a wrong tz/dow/hour on the wire. Every instance + // runs its own update loop, so more than one instance is enough to hit it. + const std::time_t fixed_time = 1700000000; + const std::tm expected_local = utils::localTime(fixed_time); + const std::tm expected_gm = utils::gmTime(fixed_time); + + std::atomic mismatches(0); + std::vector workers; + for (int t = 0; t < 8; t++) { + const bool use_local = (t % 2) == 0; + workers.emplace_back([use_local, fixed_time, &expected_local, &expected_gm, &mismatches]() { + for (int i = 0; i < 2000; i++) { + const std::tm actual = use_local ? utils::localTime(fixed_time) : utils::gmTime(fixed_time); + const std::tm &expected = use_local ? expected_local : expected_gm; + if (actual.tm_hour != expected.tm_hour || actual.tm_min != expected.tm_min || actual.tm_wday != expected.tm_wday || actual.tm_mday != expected.tm_mday) { + mismatches.fetch_add(1); + } + } + }); + } + for (std::thread &worker : workers) { + worker.join(); + } + + CHECK(mismatches.load() == 0); +} + TEST_CASE("two instances produce distinct view IDs for the same view name") { clearSDK(); InstanceFixture a("APP_KEY_A", "mi-viewid-a.db"); From 972f400cce62fd7249b07ffc4ad4c2823081d5ff Mon Sep 17 00:00:00 2001 From: turtledreams Date: Sun, 9 Aug 2026 15:52:01 +0900 Subject: [PATCH 5/5] fixes --- CHANGELOG.md | 8 +- examples/example_integration.cpp | 26 +- include/countly.hpp | 48 ++-- include/countly/logger_module.hpp | 11 + include/countly/remote_config_store.hpp | 33 +++ include/countly/sqlite_utils.hpp | 53 ++++ include/countly/storage_module_memory.hpp | 10 + src/countly.cpp | 279 ++++++++++++++-------- src/logger_module.cpp | 27 +++ src/storage_module_db.cpp | 23 +- src/storage_module_memory.cpp | 56 +++-- src/views_module.cpp | 63 ++++- tests/event_queue.cpp | 57 ++++- tests/multi_instance.cpp | 50 ++-- tests/mutex_exception_safety.cpp | 63 +++++ tests/session.cpp | 47 ++++ tests/views.cpp | 65 +++++ 17 files changed, 722 insertions(+), 197 deletions(-) create mode 100644 include/countly/remote_config_store.hpp create mode 100644 include/countly/sqlite_utils.hpp diff --git a/CHANGELOG.md b/CHANGELOG.md index 8c930db4..834f8162 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,13 @@ - ! Minor breaking change ! When built with SQLite, each instance requires its own database path. A second instance claiming a path already in use logs an error and does not initialize. - Fixed the libcurl global lifecycle: `curl_global_init` now runs once per process, and cleanup no longer runs when an instance is destroyed, which could tear down networking underneath another live instance. Added `shutdownNetworking()` for hosts that load and unload the SDK without exiting. - Fixed non-unique event and view IDs: the random component of generated IDs was constant for the lifetime of the process, and on platforms with a coarse `system_clock` (Windows, ~15ms) the timestamp component did not change either, so IDs generated within one tick were identical. -- Remote config fetches now run on an owned thread that is joined when the SDK is destroyed, instead of being detached, which could leave a fetch running after the objects it used were gone. Only one fetch runs at a time per instance: a call made while a fetch is in flight is logged and ignored rather than queued, so `updateRemoteConfig`, `updateRemoteConfigFor` and `updateRemoteConfigExcept` never block the calling thread. +- Fixed a remote config fetch outliving the objects it used: the fetch no longer holds any reference to the SDK instance, only to the modules and the value store it needs, each kept alive for as long as the fetch runs. Neither the calling thread nor destruction ever waits for it. Only one fetch runs at a time per instance: a call made while a fetch is in flight is logged and ignored rather than queued, so `updateRemoteConfig`, `updateRemoteConfigFor` and `updateRemoteConfigExcept` never block the calling thread. `shutdownNetworking()` now also refuses while a fetch is in flight. +- Calling the SDK from the log callback no longer hangs or recurses without bound. Log messages raised while a thread is inside the callback are dropped, `checkRQSize()` no longer takes the instance mutex (the storage modules serialize their own access), and the event queue size getters report -1 instead of deadlocking when called from the callback. +- Fixed dropped events and requests on SQLite builds: no busy timeout was set on any database connection, so a write that overlapped a read from another thread failed immediately with `SQLITE_BUSY` and the event or request was discarded with only an error log. Every connection now waits for the lock. +- Fixed `endSession` checking whether a session was active without holding the instance mutex, so two concurrent calls could both send an `end_session` request. +- Fixed the event queue flush when the queue is emptied by another thread mid-flush: it queued a request with an empty event list, and on SQLite builds it also ran a malformed `DELETE ... WHERE evtid IN )` statement that failed with a SQLite syntax error. +- Fixed `createEventTableSchema` and `debugReturnStateOfEQ` falling off the end of a non-void function when a `std::system_error` was caught; in the first case the indeterminate value decided whether the SDK considered itself initialized. +- Fixed a data race in the views module: the open-view map and the first-view flag were kept without a lock, so opening or closing views from more than one thread could lose view events, report `start` on more than one view, or corrupt the map. View recording still happens outside that lock, so a log callback that calls back into the SDK cannot deadlock. - Fixed a data race in the `dow`, `hour` and `tz` fields of events and requests: they were derived with `std::localtime` and `std::gmtime`, which share one process-wide buffer, so concurrent recording (each instance runs its own update loop) could report another thread's or the other function's values. Both now use the reentrant variants. - Fixed a lost wakeup when stopping the periodic SDK Behavior Settings timer: the stop flag was set without holding the mutex the timer thread waits on, so the notification could be missed and the joining thread could block for up to the full four-hour update interval. diff --git a/examples/example_integration.cpp b/examples/example_integration.cpp index ff598ca7..3366b621 100644 --- a/examples/example_integration.cpp +++ b/examples/example_integration.cpp @@ -218,14 +218,9 @@ static Countly *secondaryInstance() { return second; } -// Hammers both instances from several threads at once. Each instance has its own -// lock, so the two do not contend with each other; within one instance the calls -// below are internally synchronised. -// -// Views are deliberately driven from a single thread per instance: ViewsModule -// keeps its open-view map without a lock of its own, so concurrent openView / -// closeView calls on the *same* instance are not safe. Two threads driving views -// on two *different* instances are, since the modules are per instance. +// Hammers both instances from several threads at once. Every call below is +// internally synchronised, and each instance has its own lock, so the two do not +// contend with each other either. static void stressBothInstances(int threadsPerInstance, int iterations) { Countly *second = secondaryInstance(); if (second == nullptr) { @@ -256,6 +251,11 @@ static void stressBothInstances(int threadsPerInstance, int iterations) { target->crash().addBreadcrumb(label + "-" + std::to_string(w) + "-" + std::to_string(i)); + const std::string viewId = target->views().openView("stress view " + label); + if (!viewId.empty()) { + target->views().closeViewWithID(viewId); + } + if (i % 5 == 0) { target->updateSession(); } @@ -264,16 +264,6 @@ static void stressBothInstances(int threadsPerInstance, int iterations) { } }); } - - // One view thread per instance, see the note above. - workers.emplace_back([target, label, iterations]() { - for (int i = 0; i < iterations; i++) { - const std::string viewId = target->views().openView("stress view " + label); - if (!viewId.empty()) { - target->views().closeViewWithID(viewId); - } - } - }); } for (std::thread &worker : workers) { diff --git a/include/countly.hpp b/include/countly.hpp index 1f7c52d5..335e0770 100644 --- a/include/countly.hpp +++ b/include/countly.hpp @@ -30,6 +30,7 @@ #include #include #include +#include #include #include @@ -435,24 +436,22 @@ class Countly : public cly::CountlyDelegates { void releaseDatabasePathClaim(); /** - * Joins the remote-config fetch thread if one exists. Safe to call repeatedly - * and safe to call when none is running. Must not be called while the instance - * mutex is held -- the fetch thread takes it. - */ - void joinRemoteConfigThread(); - - /** - * Starts a remote-config fetch on the single owned fetch thread. Never blocks - * the caller: if a fetch is already in flight this logs a warning and returns - * false rather than waiting, so a call from a UI thread cannot stall on an - * HTTP timeout. + * Starts a remote-config fetch on a detached thread. Never blocks: neither the + * caller (a UI thread must not stall on an HTTP timeout) nor destruction. At + * most one fetch is in flight per instance; a call made while one is running is + * logged and dropped. + * + * The thread body captures no reference to this object -- only shared_ptrs to + * the modules and the value store it needs -- so it stays valid however long it + * outlives the instance. * - * @param member: the fetch body to run * @param data: request parameters, copied into the thread + * @param merge: true merges the response into the stored values, false replaces + * them wholesale * @param caller: public method name, used in log messages * @return true if a fetch was started */ - bool startRemoteConfigThread(void (Countly::*member)(const std::map &), const std::map &data, const char *caller); + bool startRemoteConfigFetch(const std::map &data, bool merge, const char *caller); void _deleteThread(); void _sendIndependantLocationRequest(); @@ -461,14 +460,6 @@ class Countly : public cly::CountlyDelegates { bool createEventTableSchema(); #endif - /** - * Helper methods to fetch remote config from the server. - */ -#pragma region Remote_Config_Helper_Methods - void _fetchRemoteConfig(const std::map &data); - void _updateRemoteConfigWithSpecificValues(const std::map &data); -#pragma endregion Remote_Config_Helper_Methods - void _changeDeviceIdWithMerge(const std::string &value); void _changeDeviceIdWithoutMerge(const std::string &value); @@ -488,12 +479,14 @@ class Countly : public cly::CountlyDelegates { std::unique_ptr thread; - // A remote-config fetch runs on an owned thread rather than a detached one: a - // detached thread captures `this` and can outlive the instance. At most one - // fetch is in flight, so a single thread is all that is ever needed. - std::mutex remote_config_thread_mutex; - std::thread remote_config_thread; - std::atomic remote_config_fetch_running{false}; + // Remote config values, and the in-flight flag for the fetch that writes them. + // + // Held behind a shared_ptr because the fetch thread is detached: destruction + // must not wait for an HTTP round trip, so the thread can outlive this object + // and therefore must not touch it. Everything the fetch needs is reached + // through shared_ptrs it holds itself, this store included. + std::shared_ptr remote_config_store = std::make_shared(); + std::unique_ptr crash_module; std::unique_ptr views_module; @@ -523,7 +516,6 @@ class Countly : public cly::CountlyDelegates { #endif bool remote_config_enabled = false; - nlohmann::json remote_config; }; } // namespace cly #endif diff --git a/include/countly/logger_module.hpp b/include/countly/logger_module.hpp index eeef5053..bd6ba438 100644 --- a/include/countly/logger_module.hpp +++ b/include/countly/logger_module.hpp @@ -34,6 +34,17 @@ class LoggerModule { */ void log(LogLevel level, const std::string &message); + /** + * @return true when the calling thread is currently inside the integrator's log + * callback. + * + * The SDK invokes that callback from places that hold the instance mutex, so a + * method which takes that mutex cannot serve a call made from inside the + * callback -- it would deadlock. Methods that have a "cannot determine" return + * value check this and use it instead of locking. + */ + static bool isInsideCallback(); + private: class LoggerModuleImpl; std::unique_ptr impl; diff --git a/include/countly/remote_config_store.hpp b/include/countly/remote_config_store.hpp new file mode 100644 index 00000000..b6966a6e --- /dev/null +++ b/include/countly/remote_config_store.hpp @@ -0,0 +1,33 @@ +#ifndef COUNTLY_REMOTE_CONFIG_STORE_HPP_ +#define COUNTLY_REMOTE_CONFIG_STORE_HPP_ + +#include "nlohmann/json.hpp" +#include +#include + +namespace cly { + +/** + * The remote config values, plus the flag that keeps two fetches from running at + * once. + * + * This lives in its own reference-counted object rather than inside Countly + * because the fetch runs on a detached thread: destroying an instance must not + * wait for an HTTP round trip, so the thread can outlive the instance. It holds a + * shared_ptr to this store (and to the modules it uses), which is what makes + * writing the result after the owner is gone harmless instead of a + * use-after-free. + * + * Guarded by its own mutex, not the instance mutex, for the same reason: the + * instance may no longer exist. + */ +struct RemoteConfigStore { + std::mutex mutex; + nlohmann::json values = nlohmann::json::object(); + + // True from the moment a fetch is started until its thread body returns. + std::atomic fetch_running{false}; +}; + +} // namespace cly +#endif diff --git a/include/countly/sqlite_utils.hpp b/include/countly/sqlite_utils.hpp new file mode 100644 index 00000000..8ff84c2c --- /dev/null +++ b/include/countly/sqlite_utils.hpp @@ -0,0 +1,53 @@ +#ifndef COUNTLY_SQLITE_UTILS_HPP_ +#define COUNTLY_SQLITE_UTILS_HPP_ + +#ifdef COUNTLY_USE_SQLITE + +#include "sqlite3.h" +#include + +/** + * How long a connection waits for whoever holds the database lock before giving + * up with SQLITE_BUSY. + * + * Every database operation in the SDK opens its own connection, uses it and + * closes it, so the lock is only ever held for the length of one statement -- + * no HTTP call or other blocking work happens with the lock held. Three seconds + * is therefore far more than contention needs, and short enough that a genuinely + * stuck database still surfaces as an error instead of hanging. + */ +#define COUNTLY_SQLITE_BUSY_TIMEOUT_MS 3000 + +namespace cly { +namespace utils { + +/** + * Opens a SQLite database with a busy timeout set. + * + * Always use this instead of sqlite3_open. Without a busy timeout the default is + * zero: a connection that finds the database locked fails immediately with + * SQLITE_BUSY. That happens routinely, because the SDK reads the queues from one + * thread while writing them from another -- the event queue size check + * deliberately drops the instance mutex before querying -- and a failed write is + * a silently dropped event or request. + * + * @param path: database file path + * @param database: receives the connection, and must be closed by the caller + * even when this fails, exactly as with sqlite3_open + * @return the sqlite3_open result code + */ +inline int openDatabase(const std::string &path, sqlite3 **database) { + const int return_value = sqlite3_open(path.c_str(), database); + if (*database != nullptr) { + // Worth setting even when the open failed: the handle still has to be + // closed, and a later call on it should not fail for lack of a timeout. + sqlite3_busy_timeout(*database, COUNTLY_SQLITE_BUSY_TIMEOUT_MS); + } + return return_value; +} + +} // namespace utils +} // namespace cly + +#endif // COUNTLY_USE_SQLITE +#endif diff --git a/include/countly/storage_module_memory.hpp b/include/countly/storage_module_memory.hpp index 89fc3e75..116d40c4 100644 --- a/include/countly/storage_module_memory.hpp +++ b/include/countly/storage_module_memory.hpp @@ -5,12 +5,22 @@ #include "countly/storage_module_base.hpp" #include #include +#include #include #include namespace cly { class StorageModuleMemory : public StorageModuleBase { private: + // Guards request_queue and _lastUsedId. The module owns its synchronisation so + // callers do not have to hold the instance mutex just to read a queue size -- + // taking that mutex from a public getter deadlocks when an integrator calls the + // getter from their log callback, which the SDK invokes while holding it. + // + // Never held across a _logger->log() call: the callback may call back into the + // SDK, and lock order would then run storage -> instance while the rest of the + // SDK runs instance -> storage. + std::mutex _mutex; long long _lastUsedId = 0; std::deque> request_queue; diff --git a/src/countly.cpp b/src/countly.cpp index 5bf79f26..1ed69b8f 100644 --- a/src/countly.cpp +++ b/src/countly.cpp @@ -19,6 +19,7 @@ #include "countly.hpp" #ifdef COUNTLY_USE_SQLITE +#include "countly/sqlite_utils.hpp" #include "sqlite3.h" #endif @@ -44,13 +45,65 @@ std::atomic live_instance_count(0); std::mutex networking_lifecycle_mutex; /** - * Clears the in-flight flag however a remote-config fetch body exits. + * Number of remote-config fetches running on detached threads across the whole + * process. shutdownNetworking() refuses while this is non-zero: such a thread can + * outlive the instance that started it, and releasing libcurl underneath one + * would crash it. */ -struct RemoteConfigFetchGuard { - std::atomic &flag; - explicit RemoteConfigFetchGuard(std::atomic &f) : flag(f) {} - ~RemoteConfigFetchGuard() { flag.store(false); } +std::atomic inflight_remote_config_fetches(0); + +/** + * Everything a remote-config fetch needs, each piece owned by the fetch itself. + * + * The fetch runs detached, so it may still be in an HTTP call after the Countly + * instance that started it is destroyed. Holding shared_ptrs -- and no pointer to + * the instance -- is what makes that safe: nothing the body touches can be freed + * while the body runs. + * + * Note which module is absent. ConfigurationModule keeps a raw CountlyDelegates + * pointer and calls through it from its SBS timer, so a reference held here would + * postpone its destruction past ~Countly and leave that timer running against a + * destroyed instance. Its one contribution, the networking-enabled check, is made + * before the thread starts instead. + */ +struct RemoteConfigFetch { + std::shared_ptr requestModule; + std::shared_ptr requestBuilder; + std::shared_ptr logger; + std::shared_ptr store; + std::map data; + // true merges the response into the stored values, false replaces them. + bool merge = false; }; + +/** + * Body of a detached remote-config fetch. A free function rather than a Countly + * member precisely so it cannot reach the instance. + */ +void runRemoteConfigFetch(std::shared_ptr fetch) { + // Clear the in-flight flag and the process counter however this exits. + struct Guard { + std::shared_ptr fetch; + ~Guard() { + fetch->store->fetch_running.store(false); + inflight_remote_config_fetches.fetch_sub(1); + } + } guard{fetch}; + + const HTTPResponse response = fetch->requestModule->sendHTTP("/o/sdk", fetch->requestBuilder->serializeData(fetch->data)); + if (!response.success) { + return; + } + + std::lock_guard lk(fetch->store->mutex); + if (fetch->merge) { + for (auto it = response.data.begin(); it != response.data.end(); ++it) { + fetch->store->values[it.key()] = it.value(); + } + } else { + fetch->store->values = response.data; + } +} } // namespace Countly::Countly() { @@ -66,10 +119,10 @@ Countly::Countly() { Countly::~Countly() { is_being_disposed = true; - // Before stop(), because the fetch bodies use requestModule and the instance - // mutex, both of which must still be alive. The SBS timer is joined later by - // configurationModule.reset(), exactly as it was before multi-instance. - joinRemoteConfigThread(); + // Deliberately does not wait for an in-flight remote-config fetch: that would + // make destruction block for an HTTP round trip. The fetch runs detached and + // holds its own references to everything it touches, so it stays valid on its + // own. The SBS timer is joined by configurationModule.reset() below. stop(); releaseDatabasePathClaim(); crash_module.reset(); @@ -305,18 +358,27 @@ void Countly::shutdownNetworking() { Countly *defaultInstance = registry().find(DEFAULT_INSTANCE_NAME); int live = 0; + int fetches = 0; { // Held across the check and the release so no instance can be constructed // in between and have networking torn down underneath it. std::lock_guard lk(networking_lifecycle_mutex); live = live_instance_count.load(); - if (live == 0) { + // A remote-config fetch is detached and may outlive the instance that started + // it, so an instance count of zero is not on its own proof that nobody is + // inside libcurl. + fetches = inflight_remote_config_fetches.load(); + if (live == 0 && fetches == 0) { RequestModule::releaseGlobalNetworking(); } } - if (live > 0 && defaultInstance != nullptr) { - defaultInstance->log(LogLevel::ERROR, "[Countly] shutdownNetworking, " + std::to_string(live) + " instance(s) are still live; refusing to tear down networking."); + if (defaultInstance != nullptr) { + if (live > 0) { + defaultInstance->log(LogLevel::ERROR, "[Countly] shutdownNetworking, " + std::to_string(live) + " instance(s) are still live; refusing to tear down networking."); + } else if (fetches > 0) { + defaultInstance->log(LogLevel::ERROR, "[Countly] shutdownNetworking, " + std::to_string(fetches) + " remote config fetch(es) are still in flight; refusing to tear down networking."); + } } } @@ -920,36 +982,37 @@ void Countly::startOnCloud(const std::string &app_key) { this->start(app_key, "https://cloud.count.ly", 443); } -void Countly::joinRemoteConfigThread() { - std::lock_guard lk(remote_config_thread_mutex); - if (remote_config_thread.joinable()) { - try { - remote_config_thread.join(); - } catch (const std::system_error &e) { - log(LogLevel::WARNING, std::string("[Countly] joinRemoteConfigThread, Could not join thread: ") + e.what()); - } +bool Countly::startRemoteConfigFetch(const std::map &data, bool merge, const char *caller) { + // Checked here rather than on the fetch thread: see RemoteConfigFetch on why it + // must not hold the configuration module. + if (configurationModule->isNetworkingEnabled() == false) { + log(LogLevel::ERROR, std::string("[Countly] ") + caller + ", Error fetching remote config, networking is disabled in SBS"); + return false; } -} - -bool Countly::startRemoteConfigThread(void (Countly::*member)(const std::map &), const std::map &data, const char *caller) { - std::lock_guard lk(remote_config_thread_mutex); - if (remote_config_fetch_running.load()) { + // exchange, not load-then-store: two threads calling updateRemoteConfig at the + // same moment must not both get through. + if (remote_config_store->fetch_running.exchange(true)) { log(LogLevel::WARNING, std::string("[Countly] ") + caller + ", a remote config fetch is already in flight; ignoring this call."); return false; } - // The previous fetch has finished, so this returns immediately. Joining it is - // what lets a single std::thread member serve every fetch. - if (remote_config_thread.joinable()) { - remote_config_thread.join(); - } + std::shared_ptr fetch(new RemoteConfigFetch()); + fetch->requestModule = requestModule; + fetch->requestBuilder = requestBuilder; + fetch->logger = logger; + fetch->store = remote_config_store; + fetch->data = data; + fetch->merge = merge; - remote_config_fetch_running.store(true); + inflight_remote_config_fetches.fetch_add(1); try { - remote_config_thread = std::thread(member, this, data); + // Detached on purpose: nothing may wait for an HTTP round trip, least of all + // ~Countly. The body holds `fetch` and therefore everything it touches. + std::thread(runRemoteConfigFetch, fetch).detach(); } catch (const std::system_error &e) { - remote_config_fetch_running.store(false); + inflight_remote_config_fetches.fetch_sub(1); + remote_config_store->fetch_running.store(false); log(LogLevel::ERROR, std::string("[Countly] ") + caller + ", could not create remote config thread: " + e.what()); return false; } @@ -1225,7 +1288,7 @@ std::vector Countly::debugReturnStateOfEQ() { char **table; char *error_message; - return_value = sqlite3_open(configuration->databasePath.c_str(), &database); + return_value = cly::utils::openDatabase(configuration->databasePath, &database); if (return_value == SQLITE_OK) { std::ostringstream sql_statement_stream; sql_statement_stream << "SELECT * FROM events ORDER BY evtid ASC;"; @@ -1257,6 +1320,9 @@ std::vector Countly::debugReturnStateOfEQ() { std::ostringstream log_message; log_message << "[Countly] debugReturnStateOfEQ, error: " << e.what(); log(LogLevel::FATAL, log_message.str()); + // Without this the function falls off its end and the returned vector is + // indeterminate. + return std::vector(); } } @@ -1478,6 +1544,15 @@ void Countly::packEvents() { } void Countly::sendEventsToRQ(const nlohmann::json &events) { + // Every caller decides to send based on an event queue size it read earlier, + // outside the lock. Another thread can drain the queue in between, and then + // there is nothing to send: queueing an "events=[]" request would spend a + // request on nothing. + if (events.empty()) { + log(LogLevel::DEBUG, "[Countly] sendEventsToRQ, No events to send."); + return; + } + log(LogLevel::DEBUG, "[Countly] sendEventsToRQ, Sending events to RQ."); std::map data = {{"app_key", session_params["app_key"].get()}, {"device_id", session_params["device_id"].get()}, {"events", events.dump()}}; requestModule->addRequestToQueue(data); @@ -1493,18 +1568,26 @@ bool Countly::endSession() { log(LogLevel::ERROR, "[Countly] endSession, Session tracking is disabled in server configuration, can not end session."); return false; } - if (began_session == false) { - log(LogLevel::DEBUG, "[Countly] endSession, There is no active session to end."); - return true; - } const std::chrono::system_clock::time_point now = Countly::getTimestamp(); const auto timestamp = std::chrono::duration_cast(now.time_since_epoch()); - const auto duration = std::chrono::duration_cast(getSessionDuration(now)); // lock_guard so the mutex is released on scope exit, including the early - // return below and any exception (e.g. a json type_error from a session_params + // returns below and any exception (e.g. a json type_error from a session_params // access, or addRequestToQueue) thrown while it is held. + // + // The began_session check belongs inside this lock, together with the write + // that clears it: read outside, two concurrent calls could both see an active + // session and both queue an end_session request. stop() on one thread and a + // device id change on another is enough to get there. std::lock_guard lk(*mutex); + if (began_session == false) { + log(LogLevel::DEBUG, "[Countly] endSession, There is no active session to end."); + return true; + } + + // getSessionDuration() takes this same mutex, so compute it inline here. + const auto duration = std::chrono::duration_cast(now - last_sent_session_request); + std::map data = {{"app_key", session_params["app_key"].get()}, {"device_id", session_params["device_id"].get()}, {"session_duration", std::to_string(duration.count())}, {"timestamp", std::to_string(timestamp.count())}, {"end_session", "1"}}; if (is_being_disposed) { @@ -1529,6 +1612,14 @@ int Countly::checkEQSize() { return event_count; } + // The event queue is guarded by the instance mutex, which the SDK holds while + // it invokes the log callback, so answering a call made from inside that + // callback would deadlock. -1 is the same "cannot determine" answer this + // returns before initialization. checkRQSize() has no such restriction. + if (LoggerModule::isInsideCallback()) { + return event_count; + } + #ifdef COUNTLY_USE_SQLITE event_count = checkPersistentEQSize(); #else @@ -1545,17 +1636,24 @@ int Countly::checkRQSize() { return request_count; } - { - // serialize storage access with processQueue/addRequestToQueue - std::lock_guard lk(*mutex); - request_count = static_cast(requestModule->RQSize()); - } + // Deliberately does not take the instance mutex. The storage modules serialise + // their own access -- the in-memory one with its own lock, the SQLite one + // through its per-operation connection and busy timeout -- so the count is safe + // to read without it. Taking it here would deadlock any integrator who calls + // this from their log callback, which the SDK invokes from places that hold it. + request_count = static_cast(requestModule->RQSize()); return request_count; } #ifndef COUNTLY_USE_SQLITE int Countly::checkMemoryEQSize() { log(LogLevel::DEBUG, "[Countly] checkMemoryEQSize, Checking event queue size in memory."); + // See checkEQSize(): locking here would deadlock a call made from inside the + // integrator's log callback. + if (LoggerModule::isInsideCallback()) { + return -1; + } + int result = 0; std::lock_guard lk(*mutex); result = static_cast(event_queue.size()); @@ -1568,12 +1666,21 @@ int Countly::checkMemoryEQSize() { void Countly::removeEventWithId(const std::string &event_ids) { // TODO: Check if we should check database_path set or not log(LogLevel::DEBUG, "[Countly] removeEventWithId, Removing events from storage: [" + event_ids + "]"); + + // fillEventsIntoJson() hands back ")" rather than "()" when it found no rows, + // and either one builds a malformed "... evtid IN )" statement. Nothing to + // delete in that case anyway. + if (event_ids.empty() || event_ids.find_first_of("0123456789") == std::string::npos) { + log(LogLevel::DEBUG, "[Countly] removeEventWithId, No event ids given, nothing to remove."); + return; + } + sqlite3 *database; int return_value; char *error_message; // we attempt to clear the events in the database only if there were any events collected previously - return_value = sqlite3_open(database_path.c_str(), &database); + return_value = cly::utils::openDatabase(database_path, &database); if (return_value == SQLITE_OK) { std::ostringstream sql_statement_stream; sql_statement_stream << "DELETE FROM events WHERE evtid IN " << event_ids << ';'; @@ -1610,7 +1717,7 @@ void Countly::fillEventsIntoJson(nlohmann::json &events, std::string &event_ids) char *error_message; // open database - return_value = sqlite3_open(database_path.c_str(), &database); + return_value = cly::utils::openDatabase(database_path, &database); // if database opened successfully if (return_value == SQLITE_OK) { @@ -1650,6 +1757,12 @@ void Countly::fillEventsIntoJson(nlohmann::json &events, std::string &event_ids) int Countly::checkPersistentEQSize() { int result = -1; + // See checkEQSize(): locking here would deadlock a call made from inside the + // integrator's log callback. + if (LoggerModule::isInsideCallback()) { + return result; + } + std::unique_lock lk(*mutex); if (database_path.empty()) { log(LogLevel::FATAL, "[Countly] checkPersistentEQSize, SQLite database path is not set"); @@ -1657,7 +1770,7 @@ int Countly::checkPersistentEQSize() { } sqlite3 *database; - int return_value = sqlite3_open(database_path.c_str(), &database); + int return_value = cly::utils::openDatabase(database_path, &database); lk.unlock(); if (return_value == SQLITE_OK) { @@ -1692,7 +1805,7 @@ void Countly::addEventToSqlite(const cly::Event &event) { int return_value; char *error_message; - return_value = sqlite3_open(database_path.c_str(), &database); + return_value = cly::utils::openDatabase(database_path, &database); if (return_value == SQLITE_OK) { std::ostringstream sql_statement_stream; // TODO Investigate if we need to escape single quotes in serialized event @@ -1701,7 +1814,9 @@ void Countly::addEventToSqlite(const cly::Event &event) { return_value = sqlite3_exec(database, sql_statement.c_str(), nullptr, nullptr, &error_message); if (return_value != SQLITE_OK) { - log(LogLevel::ERROR, error_message); + // Say which operation failed: this event is being dropped, and a bare + // "database is locked" gives no hint of what was lost. + log(LogLevel::ERROR, "[Countly] addEventToSqlite, Event was not stored, SQLite error: " + std::string(error_message)); sqlite3_free(error_message); } } @@ -1719,7 +1834,7 @@ void Countly::clearPersistentEQ() { int return_value; char *error_message; - return_value = sqlite3_open(database_path.c_str(), &database); + return_value = cly::utils::openDatabase(database_path, &database); if (return_value == SQLITE_OK) { return_value = sqlite3_exec(database, "DELETE FROM events;", nullptr, nullptr, &error_message); if (return_value != SQLITE_OK) { @@ -1757,7 +1872,7 @@ bool Countly::createEventTableSchema() { database_path = configuration->databasePath; - return_value = sqlite3_open(database_path.c_str(), &database); + return_value = cly::utils::openDatabase(database_path, &database); if (return_value == SQLITE_OK) { return_value = sqlite3_exec(database, "CREATE TABLE IF NOT EXISTS events (evtid INTEGER PRIMARY KEY, event TEXT)", nullptr, nullptr, &error_message); if (return_value != SQLITE_OK) { @@ -1777,6 +1892,10 @@ bool Countly::createEventTableSchema() { std::ostringstream log_message; log_message << "[Countly] createEventTableSchema, error: " << e.what(); log(LogLevel::FATAL, log_message.str()); + // Without this the function falls off its end and hands an indeterminate + // value to is_sdk_initialized, so a database error could leave the SDK + // reporting itself as initialized. + return false; } } #endif @@ -1885,21 +2004,6 @@ void Countly::enableRemoteConfig() { remote_config_enabled = true; } -void Countly::_fetchRemoteConfig(const std::map &data) { - RemoteConfigFetchGuard fetch_guard(remote_config_fetch_running); - - if (configurationModule->isNetworkingEnabled() == false) { - log(LogLevel::ERROR, "[Countly] _fetchRemoteConfig, Error fetching remote config, networking is disabled in SBS"); - return; - } - - HTTPResponse response = requestModule->sendHTTP("/o/sdk", requestBuilder->serializeData(data)); - std::lock_guard lk(*mutex); - if (response.success) { - remote_config = response.data; - } -} - void Countly::updateRemoteConfig() { if (!is_sdk_initialized) { log(LogLevel::WARNING, "[Countly] updateRemoteConfig, SDK is not initialized."); @@ -1915,34 +2019,19 @@ void Countly::updateRemoteConfig() { lk.unlock(); - // Fetch remote config asynchronously on the owned fetch thread. Never blocks: - // if a fetch is already running this logs and returns. - startRemoteConfigThread(&Countly::_fetchRemoteConfig, data, "updateRemoteConfig"); + // Asynchronous and detached: neither this call nor destruction waits for it. + // Replaces the stored values wholesale, since this fetches all of them. + startRemoteConfigFetch(data, false, "updateRemoteConfig"); } nlohmann::json Countly::getRemoteConfigValue(const std::string &key) { - std::lock_guard lk(*mutex); - nlohmann::json value = remote_config[key]; + // The store's own mutex, not the instance mutex: the fetch that writes these + // values may outlive the instance, so the two cannot share a lock. + std::lock_guard lk(remote_config_store->mutex); + nlohmann::json value = remote_config_store->values[key]; return value; } -void Countly::_updateRemoteConfigWithSpecificValues(const std::map &data) { - RemoteConfigFetchGuard fetch_guard(remote_config_fetch_running); - - if (configurationModule->isNetworkingEnabled() == false) { - log(LogLevel::ERROR, "[Countly] _updateRemoteConfigWithSpecificValues, Error fetching remote config, networking is disabled in SBS"); - return; - } - - HTTPResponse response = requestModule->sendHTTP("/o/sdk", requestBuilder->serializeData(data)); - std::lock_guard lk(*mutex); - if (response.success) { - for (auto it = response.data.begin(); it != response.data.end(); ++it) { - remote_config[it.key()] = it.value(); - } - } -} - void Countly::updateRemoteConfigFor(std::string *keys, size_t key_count) { if (!is_sdk_initialized) { log(LogLevel::WARNING, "[Countly] updateRemoteConfigFor, SDK is not initialized."); @@ -1960,9 +2049,9 @@ void Countly::updateRemoteConfigFor(std::string *keys, size_t key_count) { } lk.unlock(); - // Fetch remote config asynchronously on the owned fetch thread. Never blocks: - // if a fetch is already running this logs and returns. - startRemoteConfigThread(&Countly::_updateRemoteConfigWithSpecificValues, data, "updateRemoteConfigFor"); + // Asynchronous and detached: neither this call nor destruction waits for it. + // Merges into the stored values, since this fetches only some of them. + startRemoteConfigFetch(data, true, "updateRemoteConfigFor"); } void Countly::updateRemoteConfigExcept(std::string *keys, size_t key_count) { @@ -1982,8 +2071,8 @@ void Countly::updateRemoteConfigExcept(std::string *keys, size_t key_count) { } lk.unlock(); - // Fetch remote config asynchronously on the owned fetch thread. Never blocks: - // if a fetch is already running this logs and returns. - startRemoteConfigThread(&Countly::_updateRemoteConfigWithSpecificValues, data, "updateRemoteConfigExcept"); + // Asynchronous and detached: neither this call nor destruction waits for it. + // Merges into the stored values, since this fetches only some of them. + startRemoteConfigFetch(data, true, "updateRemoteConfigExcept"); } } // namespace cly diff --git a/src/logger_module.cpp b/src/logger_module.cpp index c5187ac7..131a25e8 100644 --- a/src/logger_module.cpp +++ b/src/logger_module.cpp @@ -4,6 +4,17 @@ #include #include namespace cly { +namespace { +/** + * True while this thread is inside the integrator's log callback. + * + * Not per instance on purpose: re-entry has to be detected wherever it comes + * from, and a callback registered on two instances could otherwise bounce between + * them. + */ +thread_local bool inside_log_callback = false; +} // namespace + class LoggerModule::LoggerModuleImpl { public: LoggerModuleImpl() {} @@ -38,6 +49,15 @@ void LoggerModule::log(LogLevel level, const std::string &message) { return; } + // This thread is already inside the callback, which has called back into the + // SDK. Almost every SDK method logs, so delivering this message would re-enter + // the callback, which would call in again: unbounded recursion ending in a + // stack overflow. Dropping it is what makes calling the SDK from a log callback + // survivable. + if (inside_log_callback) { + return; + } + // Copy the callback under the lock and invoke it outside: the callback belongs // to the integrator and may call back into the SDK, which would re-enter this // function and deadlock on a non-recursive mutex. @@ -48,7 +68,14 @@ void LoggerModule::log(LogLevel level, const std::string &message) { } if (callback != nullptr) { + // Cleared however the callback exits, including by throwing. + struct FlagGuard { + ~FlagGuard() { inside_log_callback = false; } + } guard; + inside_log_callback = true; callback(level, message); } } + +bool LoggerModule::isInsideCallback() { return inside_log_callback; } } // namespace cly diff --git a/src/storage_module_db.cpp b/src/storage_module_db.cpp index 84d9ac86..6c3aee01 100644 --- a/src/storage_module_db.cpp +++ b/src/storage_module_db.cpp @@ -3,6 +3,7 @@ #include "countly/countly_configuration.hpp" #include "countly/logger_module.hpp" #ifdef COUNTLY_USE_SQLITE +#include "countly/sqlite_utils.hpp" #include "sqlite3.h" #endif #include @@ -53,7 +54,7 @@ void StorageModuleDB::vacuumDatabase() { sqlite3 *database; int return_value; char *error_message; - return_value = sqlite3_open(_configuration->databasePath.c_str(), &database); + return_value = cly::utils::openDatabase(_configuration->databasePath, &database); if (return_value == SQLITE_OK) { return_value = sqlite3_exec(database, "VACUUM", nullptr, nullptr, &error_message); if (return_value != SQLITE_OK) { @@ -86,7 +87,7 @@ bool StorageModuleDB::createSchema(const char tableName[], const char keyColumnN char *error_message; // Open the SQLite database - return_value = sqlite3_open(_configuration->databasePath.c_str(), &database); + return_value = cly::utils::openDatabase(_configuration->databasePath, &database); if (return_value == SQLITE_OK) { // Create the table if it does not exist std::ostringstream sql_statement_stream; @@ -136,7 +137,7 @@ void StorageModuleDB::RQRemoveFront() { int return_value; char *error_message; // Open the SQLite database - return_value = sqlite3_open(_configuration->databasePath.c_str(), &database); + return_value = cly::utils::openDatabase(_configuration->databasePath, &database); if (return_value == SQLITE_OK) { // Check if the SQL statement execution is successful // Remove the first entry in the requests table std::ostringstream sql_statement_stream; @@ -184,7 +185,7 @@ void StorageModuleDB::RQRemoveFront(std::shared_ptr request) { int return_value; char *error_message; // Open the SQLite database - return_value = sqlite3_open(_configuration->databasePath.c_str(), &database); + return_value = cly::utils::openDatabase(_configuration->databasePath, &database); if (return_value == SQLITE_OK) { // Build SQL statement to remove request from database std::ostringstream sql_statement_stream; @@ -229,7 +230,7 @@ long long StorageModuleDB::RQCount() { char *error_message; // Open the SQLite database - return_value = sqlite3_open(_configuration->databasePath.c_str(), &database); + return_value = cly::utils::openDatabase(_configuration->databasePath, &database); if (return_value == SQLITE_OK) { // Define the SQL statement for counting the number of rows in the requests table std::ostringstream sql_statement_stream; @@ -282,7 +283,7 @@ std::vector> StorageModuleDB::RQPeekAll() { char *error_message; // Open the database - return_value = sqlite3_open(_configuration->databasePath.c_str(), &database); + return_value = cly::utils::openDatabase(_configuration->databasePath, &database); if (return_value == SQLITE_OK) { std::ostringstream sql_statement_stream; sql_statement_stream << "SELECT * FROM " << REQUESTS_TABLE_NAME << " ORDER BY " << REQUESTS_TABLE_REQUEST_ID << " ASC;"; @@ -344,7 +345,7 @@ void StorageModuleDB::RQInsertAtEnd(const std::string &request) { char *error_message; // Opens the database connection - return_value = sqlite3_open(_configuration->databasePath.c_str(), &database); + return_value = cly::utils::openDatabase(_configuration->databasePath, &database); if (return_value == SQLITE_OK) { // Prepares the SQL statement for inserting the request into the database std::ostringstream sql_statement_stream; @@ -381,7 +382,7 @@ void StorageModuleDB::RQClearAll() { int return_value; char *error_message; // Open database connection - return_value = sqlite3_open(_configuration->databasePath.c_str(), &database); + return_value = cly::utils::openDatabase(_configuration->databasePath, &database); if (return_value == SQLITE_OK) { std::ostringstream sql_statement_stream; sql_statement_stream << "DELETE FROM " << REQUESTS_TABLE_NAME << ";"; @@ -422,7 +423,7 @@ const std::shared_ptr StorageModuleDB::RQPeekFront() { char *error_message; // Open the SQLite database - return_value = sqlite3_open(_configuration->databasePath.c_str(), &database); + return_value = cly::utils::openDatabase(_configuration->databasePath, &database); if (return_value == SQLITE_OK) { // Construct an SQL statement to retrieve the first row of the requests table std::ostringstream sql_statement_stream; @@ -480,7 +481,7 @@ void StorageModuleDB::storeSDKBehaviorSettings(const std::string &sdk_behavior_s sqlite3 *db = nullptr; sqlite3_stmt *stmt = nullptr; - if (sqlite3_open(_configuration->databasePath.c_str(), &db) != SQLITE_OK) { + if (cly::utils::openDatabase(_configuration->databasePath, &db) != SQLITE_OK) { _logger->log(LogLevel::ERROR, "[Countly] [StorageModuleDB] storeSDKBehaviorSettings, Failed to open database"); return; } @@ -522,7 +523,7 @@ std::string StorageModuleDB::getSDKBehaviorSettings() { sqlite3_stmt *stmt = nullptr; std::string result; - if (sqlite3_open(_configuration->databasePath.c_str(), &db) != SQLITE_OK) { + if (cly::utils::openDatabase(_configuration->databasePath, &db) != SQLITE_OK) { _logger->log(LogLevel::ERROR, "[Countly] [StorageModuleDB] getSDKBehaviorSettings, Failed to open database"); return ""; } diff --git a/src/storage_module_memory.cpp b/src/storage_module_memory.cpp index 74ef3f73..5b60afb3 100644 --- a/src/storage_module_memory.cpp +++ b/src/storage_module_memory.cpp @@ -3,6 +3,7 @@ #include "countly/countly_configuration.hpp" #include "countly/logger_module.hpp" #include +#include namespace cly { StorageModuleMemory::StorageModuleMemory(std::shared_ptr config, std::shared_ptr logger) : StorageModuleBase(config, logger) {} @@ -24,8 +25,11 @@ void StorageModuleMemory::RQRemoveFront() { } _logger->log(LogLevel::DEBUG, "[Countly] [StorageModuleMemory] RQRemoveFront, Start"); - if (request_queue.size() > 0) { - request_queue.pop_front(); + { + std::lock_guard lk(_mutex); + if (request_queue.size() > 0) { + request_queue.pop_front(); + } } } @@ -40,9 +44,19 @@ void StorageModuleMemory::RQRemoveFront(std::shared_ptr request) { return; } - if (request_queue.size() > 0 && request->getId() == request_queue.front()->getId()) { - _logger->log(LogLevel::DEBUG, "[Countly] [StorageModuleMemory] RQRemoveFront, Removing request = [" + request->getData() + "]"); - request_queue.pop_front(); + // The removed entry is captured under the lock and logged after it, so the log + // callback never runs with the queue locked. + std::shared_ptr removed; + { + std::lock_guard lk(_mutex); + if (request_queue.size() > 0 && request->getId() == request_queue.front()->getId()) { + removed = request_queue.front(); + request_queue.pop_front(); + } + } + + if (removed != nullptr) { + _logger->log(LogLevel::DEBUG, "[Countly] [StorageModuleMemory] RQRemoveFront, Removing request = [" + removed->getData() + "]"); } } @@ -52,7 +66,11 @@ long long StorageModuleMemory::RQCount() { return -1; } - long long size = request_queue.size(); + long long size = 0; + { + std::lock_guard lk(_mutex); + size = request_queue.size(); + } _logger->log(LogLevel::DEBUG, "[Countly] [StorageModuleMemory] RQCount, size = [" + std::to_string(size) + "]"); return size; } @@ -65,6 +83,7 @@ void StorageModuleMemory::RQInsertAtEnd(const std::string &request) { _logger->log(LogLevel::DEBUG, "[Countly] [StorageModuleMemory] RQInsertAtEnd, request = [" + request + "]"); if (request != "") { + std::lock_guard lk(_mutex); if (request_queue.empty()) { // Since the DB (Sqlite) storage module reset the primary key when all rows get deleted. To sync with the DB storage module, the memory storage module also reset '_lastUsedId' when the request queue is empty. _lastUsedId = 1; @@ -72,8 +91,7 @@ void StorageModuleMemory::RQInsertAtEnd(const std::string &request) { _lastUsedId += 1; } - std::shared_ptr entry = std::make_shared(_lastUsedId, request); - entry.reset(new DataEntry(_lastUsedId, request)); + std::shared_ptr entry(new DataEntry(_lastUsedId, request)); request_queue.push_back(entry); } else { _logger->log(LogLevel::WARNING, "[Countly] [StorageModuleMemory] RQInsertAtEnd, request is empty"); @@ -87,12 +105,8 @@ std::vector> StorageModuleMemory::RQPeekAll() { } _logger->log(LogLevel::DEBUG, "[Countly] [StorageModuleMemory] RQPeekAll, Start"); - int qSize = request_queue.size(); - std::vector> v(qSize); - for (int i = 0; i < request_queue.size(); ++i) { - v[i] = request_queue.at(i); - } - + std::lock_guard lk(_mutex); + std::vector> v(request_queue.begin(), request_queue.end()); return v; } @@ -103,6 +117,7 @@ void StorageModuleMemory::RQClearAll() { } _logger->log(LogLevel::DEBUG, "[Countly] [StorageModuleMemory] RQClearAll, Start"); + std::lock_guard lk(_mutex); request_queue.clear(); } @@ -123,8 +138,15 @@ const std::shared_ptr StorageModuleMemory::RQPeekFront() { return front; } - if (request_queue.size() > 0) { - front = request_queue.front(); + { + std::lock_guard lk(_mutex); + if (request_queue.size() > 0) { + front = request_queue.front(); + } + } + + // Logged outside the lock, see the note on _mutex. + if (front != nullptr) { _logger->log(LogLevel::DEBUG, "[Countly] [StorageModuleMemory] RQPeekFront, request = [" + front->getData() + "]"); } else { front.reset(new DataEntry(-1, "")); @@ -133,4 +155,4 @@ const std::shared_ptr StorageModuleMemory::RQPeekFront() { return front; } -}; // namespace cly \ No newline at end of file +}; // namespace cly diff --git a/src/views_module.cpp b/src/views_module.cpp index c77c863f..9302bc98 100644 --- a/src/views_module.cpp +++ b/src/views_module.cpp @@ -3,6 +3,7 @@ #include "countly/internal_limits.hpp" #include +#include #define CLY_VIEW_KEY "[CLY]_view" @@ -16,11 +17,16 @@ class ViewsModule::ViewModuleImpl { }; private: + // Guards _isFirstView and _viewsStartTime. Views are opened and closed from + // whichever thread the integrator uses, and nothing else in the SDK + // serialises those calls, so the module has to do it itself. + std::mutex _views_mutex; bool _isFirstView = true; std::map> _viewsStartTime; cly::CountlyDelegates *_cly; + // Call with _views_mutex held. std::shared_ptr findViewByName(const std::string &name) { for (auto &x : _viewsStartTime) { if (x.second->name == name) { @@ -30,14 +36,24 @@ class ViewsModule::ViewModuleImpl { return nullptr; } - void _recordView(std::shared_ptr v, const std::map &segmentation, bool isOpenView) { + + /** + * Call with _views_mutex NOT held. This calls back into Countly::addEvent, + * which takes the instance mutex and can invoke the integrator's log callback, + * and that callback is free to call back into this module. Holding the view + * lock across it would risk a deadlock, so the shared state is read and + * updated before this runs and the outcome is passed in. + * + * @param isFirstView: only meaningful when isOpenView is true + */ + void _recordView(std::shared_ptr v, const std::map &segmentation, bool isOpenView, bool isFirstView = false) { double duration = 0; std::map viewSegments; if (isOpenView) { viewSegments["visit"] = "1"; - if (_isFirstView) { + if (isFirstView) { viewSegments["start"] = "1"; } @@ -61,11 +77,6 @@ class ViewsModule::ViewModuleImpl { viewSegments["name"] = v->name; _cly->RecordEvent(CLY_VIEW_KEY, viewSegments, 1, 0, duration); - if (isOpenView) { - _isFirstView = false; - } else { - _viewsStartTime.erase(v->viewId); - } } public: @@ -100,9 +111,17 @@ class ViewsModule::ViewModuleImpl { std::shared_ptr ptr(v); - _viewsStartTime[ptr->viewId] = ptr; + bool isFirstView = false; + { + std::lock_guard lk(_views_mutex); + _viewsStartTime[ptr->viewId] = ptr; + // Claimed under the lock, so 'start' goes on exactly one view even when + // two threads open their first view at the same time. + isFirstView = _isFirstView; + _isFirstView = false; + } - _recordView(ptr, limitedSeg, true); + _recordView(ptr, limitedSeg, true, isFirstView); return ptr->viewId; } @@ -116,7 +135,17 @@ class ViewsModule::ViewModuleImpl { _logger->log(LogLevel::WARNING, "[Countly] [ViewsModule] _closeViewWithName, ConfigurationProvider unavailable."); return; } - std::shared_ptr v = findViewByName(name); + // The view is taken out of the map under the lock, so two threads closing + // the same view cannot both record it. + std::shared_ptr v; + { + std::lock_guard lk(_views_mutex); + v = findViewByName(name); + if (v != nullptr) { + _viewsStartTime.erase(v->viewId); + } + } + if (v == nullptr) { _logger->log(cly::LogLevel::WARNING, cly::utils::format_string("[Countly] [ViewsModule] _closeViewWithName, Couldn't find " "view with name = [%s]", @@ -137,14 +166,24 @@ class ViewsModule::ViewModuleImpl { return; } - if (_viewsStartTime.find(viewId) == _viewsStartTime.end()) { + std::shared_ptr v; + { + std::lock_guard lk(_views_mutex); + std::map>::iterator it = _viewsStartTime.find(viewId); + if (it != _viewsStartTime.end()) { + v = it->second; + _viewsStartTime.erase(it); + } + } + + if (v == nullptr) { _logger->log(cly::LogLevel::WARNING, cly::utils::format_string("[Countly] [ViewsModule] _closeViewWithID, Couldn't find " "view with viewId = [%s]", viewId.c_str())); return; } - _recordView(_viewsStartTime[viewId], {}, false); + _recordView(v, {}, false); } }; diff --git a/tests/event_queue.cpp b/tests/event_queue.cpp index 1d8c96cf..a8fce948 100644 --- a/tests/event_queue.cpp +++ b/tests/event_queue.cpp @@ -4,6 +4,8 @@ #include #include #include +#include +#include #include "doctest.h" @@ -351,4 +353,57 @@ TEST_CASE("Tests that saving user details trigger flushing EQ"){ nlohmann::json customUserDetailsJson = nlohmann::json::parse(customUserDetails.data["user_details"]); CHECK(customUserDetailsJson["custom"]["custom_key"] == "custom_value"); } -} \ No newline at end of file +} +/** + * Both queues are written from whatever thread the integrator records on, plus + * the SDK's own update loop. On SQLite builds every operation opens its own + * connection, so a write that overlaps a read used to fail with SQLITE_BUSY and + * the event or request was dropped with only an ERROR log (no busy timeout was + * set). This asserts that nothing is lost when several threads record at once. + */ +TEST_CASE("concurrent recording does not lose queue writes") { + clearSDK(); + Countly &countly = Countly::getInstance(); + // A client that never succeeds. start() runs the update loop regardless of its + // start_thread argument, and a delivered request is removed from the queue, so + // without this the counts below would depend on how many loop passes happened. + countly.setHTTPClient([](bool use_post, const std::string &url, const std::string &data) { + (void)use_post; + (void)url; + (void)data; + HTTPResponse response{false, nlohmann::json::object()}; + return response; + }); + countly.setDeviceID(COUNTLY_TEST_DEVICE_ID); + countly.SetPath(TEST_DATABASE_NAME); + countly.start(COUNTLY_TEST_APP_KEY, COUNTLY_TEST_HOST, COUNTLY_TEST_PORT, false); + // Keep every event in the event queue so it can all be counted. + countly.setEventsToRQThreshold(10000); + + const int thread_count = 6; + const int per_thread = 40; + const int rq_before = countly.checkRQSize(); + REQUIRE(rq_before >= 0); + + std::vector workers; + for (int t = 0; t < thread_count; t++) { + workers.emplace_back([&countly, t]() { + for (int i = 0; i < per_thread; i++) { + countly.addEvent(cly::Event("concurrent_event_" + std::to_string(t), 1)); + // Exercises the request queue on the same database file. + std::map crashMetrics = {{"_app_version", "1.0"}, {"_os", "test"}}; + countly.crash().recordException("boom", "line1\nline2", false, crashMetrics, {}); + // Reads the event queue while the other threads write it. On SQLite this + // is the overlap that produced the dropped writes: the size check drops + // the instance mutex before querying. + countly.checkEQSize(); + } + }); + } + for (std::thread &worker : workers) { + worker.join(); + } + + CHECK(countly.checkEQSize() == thread_count * per_thread); + CHECK(countly.checkRQSize() == rq_before + thread_count * per_thread); +} diff --git a/tests/multi_instance.cpp b/tests/multi_instance.cpp index dc4b3764..6a7a2007 100644 --- a/tests/multi_instance.cpp +++ b/tests/multi_instance.cpp @@ -318,25 +318,31 @@ TEST_CASE("shutdownNetworking refuses while an instance is live") { CHECK(a.initialized()); } -TEST_CASE("a remote config fetch thread is joined by destruction") { +TEST_CASE("destruction does not wait for an in-flight remote config fetch") { clearSDK(); + static std::atomic fetch_entered(false); static std::atomic fetch_completed(false); + static std::atomic release_fetch(false); + fetch_entered.store(false); fetch_completed.store(false); + release_fetch.store(false); std::shared_ptr sdk = std::make_shared(); - // A slow client: if the fetch thread is detached, destruction returns before - // the store below runs, which is exactly the bug this asserts against. + // The fetch parks until this test releases it, so it is guaranteed to still be + // in flight while the instance is destroyed. sdk->setHTTPClient([](bool use_post, const std::string &url, const std::string &data) { (void)use_post; (void)url; cly::HTTPResponse response; response.success = true; response.data = nlohmann::json::object(); - // Only the remote-config fetch is slowed down. The SDK Behavior Settings - // fetch also targets /o/sdk (with method=sc), and delaying that one too - // would let the SBS thread set the flag and make this test meaningless. + // Only the remote-config fetch is held. The SDK Behavior Settings fetch also + // targets /o/sdk (with method=sc) and must not be blocked. if (data.find("fetch_remote_config") != std::string::npos) { - std::this_thread::sleep_for(std::chrono::milliseconds(250)); + fetch_entered.store(true); + while (!release_fetch.load()) { + std::this_thread::sleep_for(std::chrono::milliseconds(5)); + } fetch_completed.store(true); } return response; @@ -344,21 +350,37 @@ TEST_CASE("a remote config fetch thread is joined by destruction") { sdk->setDeviceID(COUNTLY_TEST_DEVICE_ID); sdk->SetPath("mi-remote-config.db"); sdk->disableSDKBehaviorSettingsUpdates(); // no periodic SBS thread in this test - // Without this, ~Countly blocks for up to COUNTLY_KEEPALIVE_INTERVAL (3s) - // joining the update loop, and that incidental wait is long enough for a - // *detached* fetch thread to finish -- which would make this test pass - // whether the thread is owned or not. With it, destruction returns promptly, - // so only an owned-and-joined thread can have set the flag. + // Without this, ~Countly waits up to COUNTLY_KEEPALIVE_INTERVAL (3s) for the + // update loop, which would hide the wait this test is measuring. sdk->enableImmediateRequestOnStop(); sdk->enableRemoteConfig(); sdk->start("APP_KEY_RC", COUNTLY_TEST_HOST, COUNTLY_TEST_PORT, false); REQUIRE(sdk->checkEQSize() == 0); - fetch_completed.store(false); sdk->updateRemoteConfig(); - sdk.reset(); // must join the fetch thread + for (int i = 0; i < 400 && !fetch_entered.load(); i++) { + std::this_thread::sleep_for(std::chrono::milliseconds(5)); + } + REQUIRE(fetch_entered.load()); // the fetch is parked in the HTTP client + const std::chrono::steady_clock::time_point before = std::chrono::steady_clock::now(); + sdk.reset(); + const long long elapsed_ms = std::chrono::duration_cast(std::chrono::steady_clock::now() - before).count(); + + // Destruction must not wait for the HTTP round trip. + CHECK(elapsed_ms < 500); + CHECK(fetch_completed.load() == false); + + // Let the fetch finish. It writes its result into a store it owns a reference + // to, so completing after its instance is gone must be harmless -- run under + // ASan or TSan this is what proves there is no use-after-free. + release_fetch.store(true); + for (int i = 0; i < 400 && !fetch_completed.load(); i++) { + std::this_thread::sleep_for(std::chrono::milliseconds(5)); + } CHECK(fetch_completed.load() == true); + // Give the detached thread a moment to run its epilogue before the test ends. + std::this_thread::sleep_for(std::chrono::milliseconds(50)); remove("mi-remote-config.db"); } diff --git a/tests/mutex_exception_safety.cpp b/tests/mutex_exception_safety.cpp index e1b4e88e..e4fcda2a 100644 --- a/tests/mutex_exception_safety.cpp +++ b/tests/mutex_exception_safety.cpp @@ -2,6 +2,7 @@ #include "doctest.h" #include "nlohmann/json.hpp" #include "test_utils.hpp" +#include #include #include #include @@ -126,3 +127,65 @@ TEST_CASE("mutex exception safety - throwing HTTP client keeps stop() responsive REQUIRE(returned); fut.get(); } + +/** + * Calling the SDK from inside the log callback used to hang or recurse without + * bound: the SDK invokes that callback from places that hold the instance mutex, + * and almost every SDK method logs, so a callback that calls in again re-enters + * the callback. Now log messages raised inside the callback are dropped, and the + * queue-size getters answer instead of taking the mutex. + * + * Wrapped in std::async so a regression fails on the timeout instead of wedging + * the test binary. + */ +TEST_CASE("the SDK can be called from the log callback") { + clearSDK(); + static std::atomic callback_invocations(0); + static std::atomic rq_non_negative(0); + static std::atomic eq_refusals(0); + static Countly *target = nullptr; + callback_invocations.store(0); + rq_non_negative.store(0); + eq_refusals.store(0); + + Countly &ct = Countly::getInstance(); + target = &ct; + // No recursion guard of its own on purpose: the SDK has to be the one that + // stops the recursion. + ct.setLogger([](LogLevel level, const std::string &message) { + (void)level; + (void)message; + if (target == nullptr) { + return; + } + callback_invocations.fetch_add(1); + if (target->checkRQSize() >= 0) { + rq_non_negative.fetch_add(1); + } + if (target->checkEQSize() < 0) { + eq_refusals.fetch_add(1); + } + }); + ct.setHTTPClient(test_utils::fakeSendHTTP); + ct.setDeviceID(COUNTLY_TEST_DEVICE_ID); + ct.SetPath(TEST_DATABASE_NAME); + ct.start(COUNTLY_TEST_APP_KEY, COUNTLY_TEST_HOST, COUNTLY_TEST_PORT, false); + + auto fut = std::async(std::launch::async, [&ct]() { + for (int i = 0; i < 20; i++) { + ct.addEvent(cly::Event("callback_event", 1)); + } + }); + const bool returned = fut.wait_for(std::chrono::seconds(10)) == std::future_status::ready; + REQUIRE(returned); // a deadlock or a stack overflow would show up here + fut.get(); + + CHECK(callback_invocations.load() > 0); + // checkRQSize does not take the instance mutex, so it can serve these calls. + CHECK(rq_non_negative.load() > 0); + // checkEQSize cannot, and reports "cannot determine" rather than deadlocking. + CHECK(eq_refusals.load() > 0); + + target = nullptr; + ct.setLogger(nullptr); +} diff --git a/tests/session.cpp b/tests/session.cpp index 07e7b84b..0b3fd482 100644 --- a/tests/session.cpp +++ b/tests/session.cpp @@ -1,9 +1,12 @@ +#include #include #include #include #include #include #include +#include +#include #include "doctest.h" @@ -189,3 +192,47 @@ TEST_CASE("event request unit tests") { CHECK(events.size() == 100); } } + +/** + * endSession() used to read began_session before taking the instance mutex and + * clear it after, so two concurrent calls could both see an active session and + * both queue an end_session request. stop() on one thread while another changes + * the device id is enough to get there. + */ +TEST_CASE("concurrent endSession calls end the session only once") { + clearSDK(); + Countly &countly = Countly::getInstance(); + // A client that never succeeds, so the update loop cannot remove the queued + // requests this test is counting. + countly.setHTTPClient([](bool use_post, const std::string &url, const std::string &data) { + (void)use_post; + (void)url; + (void)data; + HTTPResponse response{false, nlohmann::json::object()}; + return response; + }); + countly.setDeviceID(COUNTLY_TEST_DEVICE_ID); + countly.SetPath(TEST_DATABASE_NAME); + countly.start(COUNTLY_TEST_APP_KEY, COUNTLY_TEST_HOST, COUNTLY_TEST_PORT, false); + + const int rq_before = countly.checkRQSize(); // holds the begin_session request + REQUIRE(rq_before >= 0); + + std::vector workers; + std::atomic ended(0); + for (int t = 0; t < 4; t++) { + workers.emplace_back([&countly, &ended]() { + if (countly.endSession()) { + ended.fetch_add(1); + } + }); + } + for (std::thread &worker : workers) { + worker.join(); + } + + // All four calls report success (three of them because there was nothing left + // to end), but exactly one end_session request may be queued. + CHECK(ended.load() == 4); + CHECK(countly.checkRQSize() == rq_before + 1); +} diff --git a/tests/views.cpp b/tests/views.cpp index 7e241ff9..fecdf9e7 100644 --- a/tests/views.cpp +++ b/tests/views.cpp @@ -3,7 +3,10 @@ #include "test_utils.hpp" #include +#include +#include #include +#include using namespace cly; using namespace std::literals::chrono_literals; @@ -213,3 +216,65 @@ TEST_CASE("recording views") { } } } + +/** + * Views used to be opened and closed without any lock of their own, so two + * threads touching the same instance raced on the open-view map (and on the + * first-view flag). Asserts are collected here and checked on the main thread. + */ +TEST_CASE("views opened and closed from several threads at once") { + test_utils::clearSDK(); + Countly &ct = Countly::getInstance(); + ct.setHTTPClient(test_utils::fakeSendHTTP); + ct.SetPath(TEST_DATABASE_NAME); + ct.setDeviceID("test-device-id"); + ct.start("YOUR_APP_KEY", "https://try.count.ly", 443, false); + // Keep every view event in the event queue so all of them can be counted. + ct.setEventsToRQThreshold(10000); + REQUIRE(ct.debugReturnStateOfEQ().size() == 0); + + const int thread_count = 4; + const int views_per_thread = 20; + + std::mutex ids_mutex; + std::vector view_ids; + std::vector workers; + + for (int t = 0; t < thread_count; t++) { + workers.emplace_back([&ct, &ids_mutex, &view_ids, t]() { + for (int i = 0; i < views_per_thread; i++) { + const std::string viewId = ct.views().openView("view " + std::to_string(t)); + { + std::lock_guard lk(ids_mutex); + view_ids.push_back(viewId); + } + ct.views().closeViewWithID(viewId); + } + }); + } + for (std::thread &worker : workers) { + worker.join(); + } + + // Every open handed back a distinct, non-empty id. + CHECK(view_ids.size() == thread_count * views_per_thread); + std::set unique_ids(view_ids.begin(), view_ids.end()); + CHECK(unique_ids.size() == view_ids.size()); + CHECK(unique_ids.count("") == 0); + + // Every pair recorded exactly two view events, and 'start' was claimed once. + int view_events = 0; + int start_events = 0; + for (const std::string &raw : ct.debugReturnStateOfEQ()) { + nlohmann::json e = nlohmann::json::parse(raw); + if (e["key"].get() != "[CLY]_view") { + continue; + } + view_events++; + if (e["segmentation"].contains("start")) { + start_events++; + } + } + CHECK(view_events == 2 * thread_count * views_per_thread); + CHECK(start_events == 1); +}