From 5c580d75b1ec78a5b6b8af5a46d8cab7a6c68499 Mon Sep 17 00:00:00 2001 From: Winston Chang Date: Tue, 23 Jan 2018 21:39:59 -0600 Subject: [PATCH 1/2] Change to priority queue of pointers. Fixes #39 --- src/callback_registry.cpp | 14 ++++++++------ src/callback_registry.h | 19 +++++++++++++++++-- src/later.cpp | 4 ++-- 3 files changed, 27 insertions(+), 10 deletions(-) diff --git a/src/callback_registry.cpp b/src/callback_registry.cpp index adb7c5b..2145473 100644 --- a/src/callback_registry.cpp +++ b/src/callback_registry.cpp @@ -1,4 +1,6 @@ #include +#include +#include #include "callback_registry.h" CallbackRegistry::CallbackRegistry() : mutex(mtx_recursive), condvar(mutex) { @@ -6,7 +8,7 @@ CallbackRegistry::CallbackRegistry() : mutex(mtx_recursive), condvar(mutex) { void CallbackRegistry::add(Rcpp::Function func, double secs) { Timestamp when(secs); - Callback cb(when, func); + Callback_sp cb = boost::make_shared(when, func); Guard guard(mutex); queue.push(cb); condvar.signal(); @@ -14,7 +16,7 @@ void CallbackRegistry::add(Rcpp::Function func, double secs) { void CallbackRegistry::add(void (*func)(void*), void* data, double secs) { Timestamp when(secs); - Callback cb(when, boost::bind(func, data)); + Callback_sp cb = boost::make_shared(when, boost::bind(func, data)); Guard guard(mutex); queue.push(cb); condvar.signal(); @@ -27,7 +29,7 @@ Optional CallbackRegistry::nextTimestamp() const { if (this->queue.empty()) { return Optional(); } else { - return Optional(this->queue.top().when); + return Optional(this->queue.top()->when); } } @@ -39,12 +41,12 @@ bool CallbackRegistry::empty() const { // Returns true if the smallest timestamp exists and is not in the future. bool CallbackRegistry::due(const Timestamp& time) const { Guard guard(mutex); - return !this->queue.empty() && !(this->queue.top().when > time); + return !this->queue.empty() && !(this->queue.top()->when > time); } -std::vector CallbackRegistry::take(size_t max, const Timestamp& time) { +std::vector CallbackRegistry::take(size_t max, const Timestamp& time) { Guard guard(mutex); - std::vector results; + std::vector results; while (this->due(time) && (max <= 0 || results.size() < max)) { results.push_back(this->queue.top()); this->queue.pop(); diff --git a/src/callback_registry.h b/src/callback_registry.h index 1ca62ae..23dcfa3 100644 --- a/src/callback_registry.h +++ b/src/callback_registry.h @@ -4,6 +4,7 @@ #include #include #include +#include #include "timestamp.h" #include "optional.h" #include "threadutils.h" @@ -33,10 +34,24 @@ class Callback { Task func; }; +typedef boost::shared_ptr Callback_sp; + +template +struct pointer_greater_than { + const bool operator()(const T a, const T b) const { + return *a > *b; + } +}; + + // Stores R function callbacks, ordered by timestamp. class CallbackRegistry { private: - std::priority_queue,std::greater > queue; + // This is a priority queue of shared pointers to Callback objects. The + // reason it is not a priority_queue is because that can cause + // objects to be copied on the wrong thread, and even trigger an R GC event + // on the wrong thread. https://github.com/r-lib/later/issues/39 + std::priority_queue, pointer_greater_than > queue; mutable Mutex mutex; mutable ConditionVariable condvar; @@ -62,7 +77,7 @@ class CallbackRegistry { bool due(const Timestamp& time = Timestamp()) const; // Pop and return an ordered list of functions to execute now. - std::vector take(size_t max = -1, const Timestamp& time = Timestamp()); + std::vector take(size_t max = -1, const Timestamp& time = Timestamp()); bool wait(double timeoutSecs) const; }; diff --git a/src/later.cpp b/src/later.cpp index 2aab6df..9d53349 100644 --- a/src/later.cpp +++ b/src/later.cpp @@ -74,12 +74,12 @@ bool execCallbacks(double timeoutSecs) { while (true) { // We only take one at a time, because we don't want to lose callbacks if // one of the callbacks throws an error - std::vector callbacks = callbackRegistry.take(1, now); + std::vector callbacks = callbackRegistry.take(1, now); if (callbacks.size() == 0) { break; } // This line may throw errors! - callbacks[0](); + (*callbacks[0])(); } return true; } From 9a92b1b3185fb7b65096e541e10f971f7a82f499 Mon Sep 17 00:00:00 2001 From: Winston Chang Date: Tue, 23 Jan 2018 21:55:54 -0600 Subject: [PATCH 2/2] Update NEWS --- NEWS.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/NEWS.md b/NEWS.md index a574f08..ff520c2 100644 --- a/NEWS.md +++ b/NEWS.md @@ -8,6 +8,8 @@ * Fixed [issue #36](https://github.com/r-lib/later/issues/36): Failure to build on OS X <=10.12 (thanks @mingwandroid). [PR #21](https://github.com/r-lib/later/pull/21) +* Fixed [issue #39](https://github.com/r-lib/later/issues/39): Calling the C++ function `later::later()` from a different thread could cause an R GC event to occur on that thread, leading to memory corruption. [PR #40](https://github.com/r-lib/later/pull/40) + ## later 0.6 * Fix a hang on address sanitized (ASAN) builds of R. [Issue #16](https://github.com/r-lib/later/issues/16), [PR #17](https://github.com/r-lib/later/pull/17)